-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CS2113-W12-1] HealthMate #1
base: master
Are you sure you want to change the base?
[CS2113-W12-1] HealthMate #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good work from the team. Some minor suggestions for you to consider. Keep it up!!
* @param healthGoalInput the input health goal (e.g., WEIGHT_LOSS). | ||
*/ | ||
public void saveHealthGoal(String healthGoalInput) { | ||
assert healthGoalInput != null : "Health goal input cannot be null"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of assertion!!
rawCaloriesTarget = 88.362 + (13.397 * weight) + (4.799 * height) - (5.677 * age); | ||
} else { | ||
rawCaloriesTarget = 447.593 + (9.247 * weight) + (3.098 * height) - (4.330 * age); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider turning the magic numbers into constant as well to give it a meaning.
public static LocalDate currentDate() { | ||
return LocalDate.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try to name methods as verbs instead of noun, e.g. getCurrentData()
} catch (NoSuchElementException e) { | ||
// silent catch if existing user file contains no content | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try not to use empty catch block, you might want to implement some error catching/logging in the catch block.
private void saveMealToFile(List<Meal> meals, String fileName) { | ||
try (BufferedWriter writer = new BufferedWriter(new FileWriter(DATA_DIRECTORY + File.separator + fileName))) { | ||
for (Meal meal : meals) { | ||
writer.write(meal.toSaveString()); | ||
writer.newLine(); | ||
} | ||
} catch (IOException e) { | ||
e.printStackTrace(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is getting very long. One improvement you can consider is to extract such helper methods e.g. saving data to a file etc into a utils
class and invoke relevant methods if needed.
private static final String FORMAT = | ||
"add mealEntry {meal name from menu} | or | add mealEntry {meal name} /c{number of " + "calories}"; | ||
private static final String DESCRIPTION = "Adds a meal entry to the meal log either from a pre-existing meal in " + | ||
"the" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider splitting the line at other places instead of having a single word on one line.
public int size() { | ||
return this.mealList.size(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to name this method as verb as well like other methods, e.g. getSize()
|
||
public Meal extractMealFromUserInput(String userInput) { | ||
try { | ||
String command = "save meal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might consider extracting some string into named constant to give it some meaning.
switch (userInput) { | ||
case "bye": | ||
logger.log(Level.INFO, "User closes application"); | ||
UI.printFarewell(); | ||
break; | ||
default: | ||
try { | ||
this.multiCommandParsing(userInput, user); | ||
logger.log(Level.INFO, "User input contains more than 1 token"); | ||
} catch (ArrayIndexOutOfBoundsException a) { | ||
logger.log(Level.WARNING, "Invalid command", a); | ||
UI.printReply("Invalid command", "Retry: "); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no more potential cases to be added, you could consider just use a simple if...else...
. switch
will be more suitable if there are more cases.
import seedu.healthmate.command.Command; | ||
import seedu.healthmate.command.commands.LogMealsCommand; | ||
import seedu.healthmate.command.commands.SaveMealCommand; | ||
import seedu.healthmate.command.commands.ListCommandsCommand; | ||
import seedu.healthmate.command.commands.AddMealEntryCommand; | ||
import seedu.healthmate.command.commands.DeleteMealCommand; | ||
import seedu.healthmate.command.commands.DeleteMealEntryCommand; | ||
import seedu.healthmate.command.commands.MealMenuCommand; | ||
import seedu.healthmate.command.commands.UpdateUserDataCommand; | ||
import seedu.healthmate.command.commands.TodayCalorieProgressCommand; | ||
import seedu.healthmate.command.commands.HistoricCalorieProgressCommand; | ||
import seedu.healthmate.command.CommandMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good extraction of commands!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good attempt! Consider refining and adding more diagrams
To create or load a user profile the `User` class provides the method `checkForUserData` which loads | ||
saved user information if available from an existing file usingn the `HistoryTracker` or prompts the user | ||
to input new information for creating a new profile as shown in the sequence diagram below. | ||
![User SD](images/userSequenceDiagram.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of clarity, UI class is omitted since it is used across most classes to systematize any form of | ||
printing output to the user. | ||
|
||
![High Level CD](images/highLevelClassDiagram.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
The Meal class encapsulates the concept of a meal. As the purpose of this application | ||
is to track calorie consumption, this consists of a mandatory calorie entry. The meal's name attribute, | ||
is however an Optional<String> allowing a case, where no meaningful label can be attached to a certain consumption. | ||
![User SD](images/addMeal.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/images/addMeal.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the sequence diagram omit program execution prior to when add meal command is detected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall DG is okay good 👍🏻
docs/DeveloperGuide.md
Outdated
For the sake of clarity, UI class is omitted since it is used across most classes to systematize any form of | ||
printing output to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although I understand that you mentioned Ui class is omitted for clarity, however my modest opinion is that Ui is crucial component to be documented. would a separate UI class diagram be useful?
docs/DeveloperGuide.md
Outdated
8. Error Handling: | ||
- Try entering invalid commands or data to ensure the application handles errors gracefully and provides helpful error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be favourable to show some examples?
public double getTargetCalories(double height, double weight, boolean isMale, int age) { | ||
assert height > 0 : "Height must be positive"; | ||
assert weight > 0 : "Weight must be positive"; | ||
assert age > 0 : "Age must be positive"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to guard against negative height/weight/age !
I am wondering, would it be useful to guard against unrealistic data such as 500 yrs of Age ?
For the sake of clarity, UI class is omitted since it is used across most classes to systematize any form of | ||
printing output to the user. | ||
|
||
![High Level CD](images/highLevelClassDiagram.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good separation of concerns between classes !
|
||
#### HealthMate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear explanation of classes
|
||
HistoryTracker allows for the persistance of user inputted data between sessions by storing it in a local csv file. | ||
|
||
## Features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could some Sequence Diagrams of the processing of Commands be shown?
docs/DeveloperGuide.md
Outdated
![User SD](images/userSequenceDiagram.jpg) | ||
Reference diagrams used | ||
![User SD](images/askForUserDataSD.png) | ||
![Read User SD](images/readUserDataFromFileSD.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good decision to show two sub-diagrams for clarity; creating a new profile for new user, and loading saved file for existing user
docs/DeveloperGuide.md
Outdated
--- | ||
### Creating a User Profile | ||
To create or load a user profile the `User` class provides the method `checkForUserData` which loads | ||
saved user information if available from an existing file usingn the `HistoryTracker` or prompts the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a minor typo hehe , "using" is the correct spelling instead of "usingn"
docs/DeveloperGuide.md
Outdated
This specifically includes adherance to the "Tell Don't Ask" principle which was enforced by | ||
making most attributes of all classes above private and avoiding getter methods if possible. | ||
The following diagram illustrates the resulting associations, methods and attributes. | ||
For the sake of clarity, UI class is omitted since it is used across most classes to systematize any form of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be helpful to include a minimal/simple representation of the UI class in the high level class diagram to indicate dependency across all classes. this might improve clarity around UI interactions
To create or load a user profile the `User` class provides the method `checkForUserData` which loads | ||
saved user information if available from an existing file usingn the `HistoryTracker` or prompts the user | ||
to input new information for creating a new profile as shown in the sequence diagram below. | ||
![User SD](images/userSequenceDiagram.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with cnivedit about the Scanner instance's activation bar. additionally, if scanner is indeed reused across multiple interactions, would it make sense to show its initialisation outside the main sequence flow?
docs/DeveloperGuide.md
Outdated
The Meal class encapsulates the concept of a meal. As the purpose of this application | ||
is to track calorie consumption, this consists of a mandatory calorie entry. The meal's name attribute, | ||
is however an Optional<String> allowing a case, where no meaningful label can be attached to a certain consumption. | ||
![User SD](images/addMeal.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
to input new information for creating a new profile as shown in the sequence diagram below. | ||
![User SD](images/userSequenceDiagram.jpg) | ||
Reference diagrams used | ||
![User SD](images/askForUserDataSD.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a good DG, perhaps the Command Handling Section could do with some sequence diagrams or examples of inputs to aid clarity
docs/DeveloperGuide.md
Outdated
This specifically includes adherance to the "Tell Don't Ask" principle which was enforced by | ||
making most attributes of all classes above private and avoiding getter methods if possible. | ||
The following diagram illustrates the resulting associations, methods and attributes. | ||
For the sake of clarity, UI class is omitted since it is used across most classes to systematize any form of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise to previous reviewers, my belief is that some documentation of the UI component would be useful in order to provide clarity into possible outputs a user should receive. perhaps you could separate the UI diagram form the overarching diagram provided to retain the clarity your diagram currently has while still providing viewers with the information regarding the UI class.
- A `MealList` object called `mealOptions` | ||
- Contains meals that are presaved by the user for quick selection to track commonly consumed meals | ||
- the command `meal menu` is used to display the current mealOptions. The implementation of this command is shown in the UML diagram below. | ||
![Meal Menu SD](images/mealMenuSD.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps the diagram could include a user so that the run() function does not appear out of nowhere, providing additional clarity.
To create or load a user profile the `User` class provides the method `checkForUserData` which loads | ||
saved user information if available from an existing file usingn the `HistoryTracker` or prompts the user | ||
to input new information for creating a new profile as shown in the sequence diagram below. | ||
![User SD](images/userSequenceDiagram.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally, this image used is quite blurry and the details are hard to see regardless of zoom. perhaps the team could consider making the diagram slightly easier on the eyes to improve user experience. another possible solution could be to break the diagram into smaller independent sections. as cnivedit mentioned, the file creation may not be the mos relevant to the creation of a user profile so can consider depicting file creation in a separate diagram.
Add UserEntryList class. Change txt save file to csv.
…ations 66 add meal option recommendations
…ryry-3302/tp into 60-timeline-of-weight-history
Bug fix historicCalories Test
Fix timestamp bug
…oon2002/tp into Bug-Fixes-+-Update-Docs
Update Jonas PPP
Bug fixes + Update docs
…ssing-t-in-list-commands-documentation Fix: add documentation to add meal entry
PPP + DG changes
Fix: Renamed PPP according to submission guidelines
change team name to lowercase
Include HP intro text and PPP updates
Fix formatting UG
…ssing-t-in-list-commands-documentation Fix
Helping busy and CLI-affine people to build healthy eating habits to improve their mental and physical well-being.