Skip to content

Support Checks for Missing Chapters#1

Open
BryanHo10 wants to merge 4 commits intodevelopfrom
bh-Missingchapters
Open

Support Checks for Missing Chapters#1
BryanHo10 wants to merge 4 commits intodevelopfrom
bh-Missingchapters

Conversation

@BryanHo10
Copy link
Contributor

No description provided.

@BryanHo10 BryanHo10 requested a review from rbnswartz August 14, 2019 22:37
Copy link
Member

@rbnswartz rbnswartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're making changes to this already so just add these to the list.

/// <returns></returns>
private JObject retrieveBibleMetadata()
{
string jsonData = File.ReadAllText(@"./MetaData.json");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be configurable in some way. Generally hard-coded file names are a bad idea.

</ItemGroup>

<ItemGroup>
<None Update="MetaData.json">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metadata is metadata and not MetaData

/// Retrieves Bible metadata from local json file (MetaData.json)
/// </summary>
/// <returns></returns>
private JObject retrieveBibleMetadata()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would serialize this to an object instead of just using JObject. You know what the data is going to be like and that will remove a whole list of potential bugs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants