Skip to content

Mahajapp code review2#58

Open
pranavm7 wants to merge 2 commits intomainfrom
mahajapp_CodeReview2
Open

Mahajapp code review2#58
pranavm7 wants to merge 2 commits intomainfrom
mahajapp_CodeReview2

Conversation

@pranavm7
Copy link
Copy Markdown
Collaborator

Deny the deletions in IngredientMapService.kt as it was a fault with the rebase. However my commits are the additional comments I've added.

My code Review.

Does the program compile : circle ci fails.
Was the program available in UC Github on time: Yes
Is the program documented/commented well enough for you to understand: Insufficient, hence my suggestions are comments.

What I learned from this Code Review -

  • Resolving Merge Conflicts
  • Using git within Android Studio
  • I learned documentation using KDoc

Commits to my project:

Comment on lines +6 to +16
/**
* A class to represent recipes
*
* This class serializes the JSON from IRecipeDAO and parses it into a kotlin object
* @property recipeID
* @property name Name of the recipe
* @property category Category of the recipe
* @property cuisine Cuisine of the recipe
* @property instructions Instructions of how to create the recipe
* @property imageURL URL of the image associated with the recipe
*/
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added documentation to the data class.

Comment on lines +71 to +73
/**
* Fetches recipes from IRecipeDAO and adds it to an arraylist
*/
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added comments to provide context for the function

Comment on lines -3 to -7
/**
* Ingredient mapping service interface that has 2 functions
* [stringToMap] converts a string into a map
* [mapToString] converts a map to a string
*/
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Please ignore these deletions, seems like the merge caused accidental deletions

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.

1 participant