Skip to content

[CS2113-T11b-1] CookingAids#71

Open
jxromy wants to merge 530 commits into
nus-cs2113-AY2425S2:masterfrom
AY2425S2-CS2113-T11b-1:master
Open

[CS2113-T11b-1] CookingAids#71
jxromy wants to merge 530 commits into
nus-cs2113-AY2425S2:masterfrom
AY2425S2-CS2113-T11b-1:master

Conversation

@jxromy
Copy link
Copy Markdown

@jxromy jxromy commented Mar 11, 2025

CookingAids helps exchange students plan for healthy meals effortlessly. With an intuitive meal planner and a built-in recipe generator, users can create healthy meal plans and optimise their grocery shopping. The recipe generator suggests easy-to-make dishes based on available ingredients, making meal preparation more efficient and cost-effective.

private static int currentId = 1; // Keeps track of the latest assigned ID

public static synchronized int generateNewDishId() {
return currentId++; // Increment and return the next ID
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be nice to have comments for the reader rather than the coder to increase readability. Avoid adding comments which are for the coder's reference.

removeExpiredIngredients(name);

// Reduce quantity starting from the earliest expiry date
for (Iterator<Ingredient> iterator = ingredientList.iterator(); iterator.hasNext() && amount > 0; ) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be nice to use more intuitive variable names than "iterator" to increase code readability.


// Reduce quantity starting from the earliest expiry date
for (Iterator<Ingredient> iterator = ingredientList.iterator(); iterator.hasNext() && amount > 0; ) {
Ingredient ing = iterator.next();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly for "ing" , better to use more intuitive variable name.


ArrayList<String> ingredients = new ArrayList<>();
if (!ingredientsString.isEmpty()) {
String[] ingredientArray = ingredientsString.split(",");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try avoiding the usage of magic strings and magic literals

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for spotting this! Issue has been fixed in the latest commit

}


public static void deleteDish(String receivedText) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a very long method. try to keep method length to about 30 LOC.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Long methods have been refactored in the latest commit

Copy link
Copy Markdown

@noradazaperez noradazaperez left a comment

Choose a reason for hiding this comment

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

  1. Shouldn't you be using standard UML notation? It seems that the red squares and green circles are not standard UML.

image

  1. Maybe the classes and methods are too detailed. Try being more simple. For example, instead of
    static void initializeIngredientStorage(HashMap , List newIngredients), you could try
  • initializeIngredienteStorage(String[], Ingredient[]): void
  1. You might be missing some attributes and methods in the Ingredient class.
    image

  2. Shouldn't user stories be properly edited?
    image

  3. It seems that you guys haven't added any non -functional requirements or instructions for manual testing.

Copy link
Copy Markdown

@kaixiangg kaixiangg left a comment

Choose a reason for hiding this comment

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

Progress looks good to me so far, just a few nits to pick, keep it up! :)

Comment thread docs/DeveloperGuide.md

#### <ins>Implementation</ins>

* `Food` is an abstract class.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice job displaying the details of the classes here clearly, however would it be more concise and clear if it is integrated directly together with class diagrams?

Comment thread docs/DeveloperGuide.md Outdated
@@ -2,29 +2,144 @@

## Acknowledgements
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider including sources used here such as different libraries used.

Comment thread docs/DeveloperGuide.md Outdated


![img.png](img.png)
### UI
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider making each of the components of the application with a larger font for easier readability for developers who are collaborating

Comment thread docs/DeveloperGuide.md
| v2.1 | aspiring chef | save my favourite recipes | access them easily |
| v2.1 | user that is going to shop soon | be recommended recipes that are an item away | plan what to buy next when I'm not limited by my current ingredients |
## Non-Functional Requirements

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider adding manual testing section when one downloads the jar file for more completeness

@kaixiangg
Copy link
Copy Markdown

image
Similar to the earlier reviews left, there could be formatting issues that might not directly reflect when using the IDE and hence the markdown is not being displayed nicely.

Copy link
Copy Markdown

@jwyk jwyk left a comment

Choose a reason for hiding this comment

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

Other than rendering problems and fore-mentioned comments, the DG looks clean and good 👍.

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +117 to +121
| Version | As a ... | I can ... | So that I can ... |
|---------|----------|-----------|-------------------|
| v1.0 | Veteran cook | add new recipes to the list | enjoy my own recipes without relying on the default list |
| v1.0 | forgetful student | see my current quantity of ingredients | check if I have enough ingredients to make foods and whether I should buy more |
| v1.0 | indecisive user | change my meal plans | have flexibility in my meals |
Copy link
Copy Markdown

@jwyk jwyk Apr 2, 2025

Choose a reason for hiding this comment

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

Should align this to make the table work because the table isn't rendering properly

Comment thread docs/DeveloperGuide.md Outdated

The bulk of the app's work is done by the following components:
* `UI`: The UI of the App.
* `Command`:The command executor.
Copy link
Copy Markdown

@jwyk jwyk Apr 2, 2025

Choose a reason for hiding this comment

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

Missing whitespace.

Comment thread docs/DeveloperGuide.md Outdated

### How the architecture components interact with each other
The _Sequence Diagram_ below shows how the components interact with each other for the scenario where the user issues
the command add -ingredient=tomato -quantity=5 -expiry=2025-04-03
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should enclose this command in `` tags.

Comment thread docs/DeveloperGuide.md
Copy link
Copy Markdown

@jwyk jwyk Apr 2, 2025

Choose a reason for hiding this comment

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

Might want to change the way how you displayed the PlantUML diagrams for public/protected/private parameters and methods to follow CS2113 convention.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider increase the word font to increase readability

}

ExpiryDate --|> DishDate
Ingredient --|> Food
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could potentially include the multiplicity between food and ingredients to increase the clarity

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

could potentially show how these block of commands interact with other part of the system

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

there is a lack of the diagram to show the data flow, ie after the data is being parsed, how is it being stored/ modified. Could consider to add that in

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +116 to +123
* `AddDishCommand` adds a dish to the DishCalendar.
* `RemoveDishCommand` removes a dish from the DishCalendar.
* `AddRecipeCommand` adds a recipe to the RecipeBank.
* `RemoveRecipeCommand` removes a recipe from the RecipeBank.
* `AddIngredientCommand` adds an ingredient to the IngredientStorage.
* `RemoveIngredientCommand` removes an ingredient from the IngredientStorage.
* `AddShoppingItemCommand` adds an item to the ShoppingList.
* `RemoveShoppingItemCommand` removes an item from the ShoppingList.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will it be better to elaborate on the implementation of the commands rather than briefly stating the description?

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +131 to +132
The following is a UML diagram of the `commands` package:
![Commands.png](images/Commands.png)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Detailed and well-structured UML diagram for understanding. Good job!

Comment thread docs/DeveloperGuide.md Outdated
| v2.1 | beginner meal prepper | see meal-prepping tips | improve my meal-prepping |
| v2.1 | health conscious student | track calories of dishes | know exactly how many calories my food has |
| v2.1 | aspiring chef | save my favourite recipes | access them easily |
| v2.1 | user that is going to shop soon | be recommended recipes that are an item away | plan what to buy next when I'm not limited by my current ingredients |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very detailed and developed user stories help to visualize the developing process of your project.

Comment thread docs/DeveloperGuide.md
| v2.1 | aspiring chef | save my favourite recipes | access them easily |
| v2.1 | user that is going to shop soon | be recommended recipes that are an item away | plan what to buy next when I'm not limited by my current ingredients |
## Non-Functional Requirements

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe consider to add sequence diagram for each command you have for better visualization of your commands.

Comment thread docs/DeveloperGuide.md Outdated
Below is a Sequence Diagram of an example of how a User performs command view -month=5 to view his dishes for the month of May
![img.png](images/sequenceView.png)
The class diagrams of UI and Calendar printer are seen below
![img.png](images/classUI.png)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think there are icons used in the plantUML diagram that does not follow course guidelines.

Comment thread docs/DeveloperGuide.md
The following is the class diagram for the classes Recipe and RecipeBank.

![Items.png](images/Items.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job creating the class diagram! But I think the link between recipeBank and recipe is a bit ambiguous, there is a "1" but also a "contains" and "many".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If recipeBank contains a recipe, I think the arrow should be the other way around.

Comment thread docs/DeveloperGuide.md
The following is the class diagram for the classes Recipe and RecipeBank.

![Items.png](images/Items.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, maybe its better to add a * to the link between recipe and ingredient.

Comment thread docs/DeveloperGuide.md
The following is the class diagram for the classes Recipe and RecipeBank.

![Items.png](images/Items.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If recipeBank contains a recipe, I think the arrow should be the other way around.

Comment thread docs/DeveloperGuide.md
The following is the class diagram for the classes Recipe and RecipeBank.

![Items.png](images/Items.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The link between recipeBank and dish also has no arrow.

Comment thread docs/DeveloperGuide.md Outdated
<div style="page-break-after: always;"></div>

### How the architecture components interact with each other
The _Sequence Diagram_ below shows how the components interact with each other for the scenario where the user issues
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

class diagram might be more suitable to represent overall architecture instead of a sequence diagram

Comment thread docs/DeveloperGuide.md Outdated
These classes facilitate user interaction by providing formatted output and handling command-based operations.

Below is a Sequence Diagram of an example of how a User performs command view -month=5 to view his dishes for the month of May
![img.png](images/sequenceView.png)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would be better to standardise a colour for all your diagrams for consistency

Comment thread docs/DeveloperGuide.md
This approach keeps business logic separate from UI logic, improving code maintainability and readability.

<div style="page-break-after: always;"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can consider adding a line break between each section for better readability

Comment thread docs/DeveloperGuide.md
planning their meals with their limited budget and time.
<div style="page-break-after: always;"></div>

## User Stories
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

might want to double check the format of user stories, does not properly show up as table

Comment thread docs/DeveloperGuide.md Outdated

The following is the class diagram for the classes Recipe and RecipeBank.

![Items.png](images/Items.png)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can relook at positioning of classes to make labels on arrows more visible

Comment on lines +44 to +48
printCalendarRow(year, month, secondRowFirstDate, daysInMonth, list);
printCalendarRow(year, month, thirdRowFirstDate, daysInMonth, list);
printCalendarRow(year, month, fourthRowFirstDate, daysInMonth, list);
printCalendarRow(year, month, fifthRowFirstDate, daysInMonth, list);
printCalendarRow(year, month, sixthRowFirstDate, daysInMonth, list);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This part could probably be done using a loop

Comment on lines +16 to +17
private boolean expiringSoon = false;
private boolean expired = false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think these can be updated based on current date automatically rather than requiring separate fields

Comment on lines +184 to +190
int startIndex = receivedText.indexOf(RECIPE_FLAG) + RECIPE_FLAG.length();
int endIndex = receivedText.indexOf(" ", startIndex);
if (endIndex == -1) {
endIndex = receivedText.length();
}

return receivedText.substring(startIndex, endIndex).trim();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This code was repeated several times

@JsonProperty("name") String dishName,
@JsonProperty("date") String dishDate) {

this.dishName = dishName.toLowerCase();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great consideration of case-sensitivity!

whalesyong and others added 30 commits April 8, 2025 12:10
Add LoggerTest, update PPP
* 'master' of https://github.com/jxromy/tp:
  Update whalesyong.md
  Update AboutUs.md
  Update AboutUs.md
  Update AboutUs.md
  fixing text ui test
  minor fix for logger
  Fix Text UI for Github CI
  Robustify tests for CI
  Add debug code for Github CI
  Try fixing github CI
  Add flushing for log tests for Github CI
  Fix checkstyle AGAIN
  resolve checkstyle err
  Fix Test errors for CI
  Fix checkstlye
  Add LoggerTest, update PPP
  Add PPP segment

# Conflicts:
#	data/cookingaids.json
fix error emssages for shopping list
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.