[CS2113-F14-4] EZMealPlan#44
Conversation
Merge branch 'master' into branch-rchlai
thomasjlalba
left a comment
There was a problem hiding this comment.
Good work thus far! Just some minor suggestions on your coding standards and quality. Keep up the good work!
| */ | ||
| public Meal removeMeal(int index) throws EZMealPlanException { | ||
| try { | ||
| return mealList.remove(index - 1); |
There was a problem hiding this comment.
Maybe it would be good to comment why it is index - 1 to make it more understandable
| int indexOfIndex = 1; | ||
|
|
||
| try { | ||
| return input.split("\\s+")[indexOfIndex]; |
There was a problem hiding this comment.
Maybe it would be good to refactor the regex to make it more understandable to readers and to avoid magic strings
| if (file.exists()) { | ||
| Scanner scanner = new Scanner(file); | ||
| while (scanner.hasNextLine()) { | ||
| String line = scanner.nextLine().trim(); | ||
| if (line.isEmpty()) { | ||
| continue; | ||
| } | ||
| // Split the line by "|" with optional spaces around it. | ||
| String[] parts = line.split("\\s*\\|\\s*"); | ||
| int minLengthToHaveIng = 2; | ||
| if (parts.length < minLengthToHaveIng) { | ||
| continue; // Skip lines that don't have ingredients. | ||
| } | ||
| checkValidMeal(parts, meals); | ||
| } | ||
| scanner.close(); | ||
| } |
There was a problem hiding this comment.
Maybe to reduce the nesting you can look into using a guard clause:
if (!file.exists()) {
return meals;
}
// if file exists
| } | ||
| // Split the line by "|" with optional spaces around it. | ||
| String[] parts = line.split("\\s*\\|\\s*"); | ||
| int minLengthToHaveIng = 2; |
There was a problem hiding this comment.
Maybe this can be placed as a constant instead?
| int openBracketIndex = ingredientStr.indexOf("("); | ||
| int closeBracketIndex = ingredientStr.indexOf(")"); | ||
| int notFoundIndex = -1; | ||
| if (openBracketIndex == notFoundIndex || closeBracketIndex == notFoundIndex) { |
There was a problem hiding this comment.
Maybe you can refactor the expression within the if statement to be a simpler expression.
| return; | ||
| } | ||
| switch(commandDescription) { | ||
| case "list": |
There was a problem hiding this comment.
Maybe you can look into refactoring this to avoid magic strings
| String splitRegex = "\\s*,\\s*"; | ||
| String[] ingredientsArray = ingInput.split(splitRegex); |
There was a problem hiding this comment.
Good work here refactoring the magic string regex, do make it consistent for the rest of the project!
| String ing = "/ing"; | ||
| String mname = "/mname"; | ||
| String mcost = "/mcost"; | ||
| String filterMethod = ""; | ||
| final String byIng = "byIng"; | ||
| final String byMname = "byMname"; | ||
| final String byMcost = "byMcost"; |
There was a problem hiding this comment.
Good work on refactoring the magic strings! But maybe since it is a constant, its naming could be in SCREAMING_SNAKE_CASE.
There was a problem hiding this comment.
Might be worth moving images into a folder ie docs/imgs, nbd tho
| assertIterableEquals(expectedMeals, testUI.capturedMeals); | ||
| } | ||
| ``` | ||
| ## Implementation |
There was a problem hiding this comment.
Could write this part or remove redundant title
| |Version| As a ... | I want to ... | So that I can ...| | ||
| |--------|----------|---------------|------------------| | ||
| |v1.0|new user|see usage instructions|refer to them when I forget how to use the application| | ||
| |v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list| |
There was a problem hiding this comment.
Maybe just attatch this part to the bottom of the above table, since you have a column for version it would be redundant if every version is a separate table
jxromy
left a comment
There was a problem hiding this comment.
Reviewed your UML diagrams and syntax, overall a well-written DG :)
| |Version| As a ... | I want to ... | So that I can ...| | ||
| |--------|----------|---------------|------------------| | ||
| |v1.0|new user|see usage instructions|refer to them when I forget how to use the application| | ||
| |v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list| |
There was a problem hiding this comment.
User stories for v1.0 looks good! perhaps more can be added to v2.0?
|  | ||
| This sequence diagram shows the procedures of extracting meals from the "mainMealFile" (mainList.txt). The procedures of extracting meals from the "userMealFile" (userList.txt) can be depicted simply by replacing "mainMealFile" with "userMealFile", storage.getMainListFile() with storage.getUserListFile(), mealManager.getMainMeals() with mealManager.getUserMeals() and lastly, "mainMeals" of MainMeals class with "userMeals" of UserMeals class. | ||
|
|
||
|  |
There was a problem hiding this comment.
Great diagram! I love the use of different colours
|
|
||
| - All user inputs are case-sensitive and normalised to lowercase. | ||
|
|
||
|  |
There was a problem hiding this comment.
Activation boxes are clearly shown and program flow is depicted well
There was a problem hiding this comment.
New line error in text after diagrams in this section
|  | ||
| This sequence diagram shows the processes that EZMealPlan system has to undergo while it is being booted up before it is ready for usage. | ||
|
|
||
|  |
There was a problem hiding this comment.
Perhaps a High-Level Architecture Diagram would help to illustrate how major components interact (e.g., Parser → Command → MealManager → Storage)
There was a problem hiding this comment.
I agree with this statement as the diagram would illustrate the relationship between each componenets as well
| * | ||
| * @param ingredients a list of ingredients (with name and price) to be bought. | ||
| */ | ||
| public BuyCommand(List<Ingredient> ingredients) { |
There was a problem hiding this comment.
dont see this command in the Dev guide.Where does this fit?Is this for constructing meals?
| public abstract class FilterSelectCommand extends Command { | ||
| String filterOrSelect; | ||
| String ing = "/ing"; | ||
| String mname = "/mname"; |
There was a problem hiding this comment.
why arent these final,dosent seem like they are getting changed in this class.
| ui.printDeleteCommandHelp(); | ||
| break; | ||
| case "view": | ||
| ui.printViewCommandHelp(); |
There was a problem hiding this comment.
good that code is being abstracted out,SLAP
| logger.fine("exiting EZMealPlan"); | ||
| } | ||
|
|
||
| private static void checkConstructedLists(MealManager mealManager) { |
There was a problem hiding this comment.
could these commands be abstracted out into their own classes/packages?
TiangSoonYong
left a comment
There was a problem hiding this comment.
Overall the UML diagrams are well constructed and readable. One minor issue is regarding the use of PNGs instead of the PUML itself.
There was a problem hiding this comment.
It is mentioned that Product is an Abstract, perhaps it should be reflected here as well. Maybe you could change the diagonal arrows to vertical arrows instead as it affected the readability of the multiplicity of ingredientList.
There was a problem hiding this comment.
I like the use of colours within the diagram and it make it more readable!
|  | ||
| This sequence diagram shows the processes that EZMealPlan system has to undergo while it is being booted up before it is ready for usage. | ||
|
|
||
|  |
There was a problem hiding this comment.
I agree with this statement as the diagram would illustrate the relationship between each componenets as well
| - **Storage**: Handles saving and loading from `main_meal_list.txt` and `user_meal_list.txt`. | ||
|
|
||
| {Describe the target user profile} | ||
| ### Logging |
There was a problem hiding this comment.
Logging section may need some more expanding on
|
|
||
| **Testability:** | ||
| - The design allows for easy unit testing by injecting a test-specific UI that captures the output. | ||
|
|
| | v1.0 | Time-constrained user | Check for recipes based on cook time | Quickly find recipes for myself | | ||
| | v1.0 | Social user | Share my favorite recipes with friends | Get feedback from friends or cooks | | ||
| | v1.0 | new user | see usage instructions | refer to them when I forget how to use the application | | ||
|
|
There was a problem hiding this comment.
user stories are well thought out and contains many considerations for the user
| assertIterableEquals(expectedMeals, testUI.capturedMeals); | ||
| } | ||
| ``` | ||
| ## Implementation |
There was a problem hiding this comment.
The implementation may need to be worked on, with the sequence diagrams added here
|  | ||
| This sequence diagram shows the procedures of extracting meals from the "mainMealFile" (mainList.txt). The procedures of extracting meals from the "userMealFile" (userList.txt) can be depicted simply by replacing "mainMealFile" with "userMealFile", storage.getMainListFile() with storage.getUserListFile(), mealManager.getMainMeals() with mealManager.getUserMeals() and lastly, "mainMeals" of MainMeals class with "userMeals" of UserMeals class. | ||
|
|
||
|  |
There was a problem hiding this comment.
Well drawn diagram! The colour coding made it easier to read.
|
|
||
| ##### 1.3 Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
The return lines should be dashed instead of solid
Update Userguide
Add InventoryCommand
update Userguide
Update DG for SelectCommand
Update the userguide
Update DeveloperGuide with CreateCommand and CreateChecker diagrams
Branch userguide v3
Add to dg Delete
Improve Test Coverage
Add more sequence diagrams
Update UML and correct typos in UG
update DG with for consume and checker
…into branch-final-refactoring # Conflicts: # src/main/java/ezmealplan/command/RecommendCommand.java
Update developer guide
…into branch-final-refactoring # Conflicts: # src/test/java/ezmealplan/command/RecommendCommandTest.java
Update ppp
Branch Final Refactoring
Update PPP
Update UG and PPP
Fix problems with DG
EZMealPlan helps busy people with food dilemma and meal-preparation issues to suggest food recommendations for them. It uses a simple-to-use CLI to provide meal suggestions to the users, taking into account their budget, food preferences and the ingredients they have.