Skip to content
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-F11-2] EssenMakanan #35

Open
wants to merge 718 commits into
base: master
Choose a base branch
from

Conversation

StanleyW00
Copy link

No description provided.

Choose a reason for hiding this comment

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

There can possibly be use of different types of UML diagrams instead of just one type.

Copy link

Choose a reason for hiding this comment

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

methods in class diagrams also have many attributes and methods, maybe can remove irrelevant methods/attributes to improve readability

@@ -14,25 +219,37 @@

{Describe the target user profile}

This product is for people who share kitchen space and ingredients with other cooks.

Choose a reason for hiding this comment

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

The target user profile could be more descriptive.

@@ -1,11 +1,216 @@
---

Choose a reason for hiding this comment

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

Maybe the use of short code snippets in the DG would be good for users to improve their understanding.


### Architecture

<img src="images/ArchitectureDiagram.png" width="280" />

Choose a reason for hiding this comment

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

The architecture diagram hyperlink does not seem to work

| v1.0 | beginner user | add new recipes into the list | learn and try out new recipes |
| v1.0 | beginner user | see all ingredient I have | |
| v1.0 | beginner user | add ingredients to the empty list | add an item to my kitchen inventory |
| v2.0a | amateur | delete some recipes | remove a recipe that I no longer want to use |

Choose a reason for hiding this comment

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

Perhaps could add more user stories for later versions


`AddIngredientCommand` will call `Ui` class to print out the name of the recently added ingredient.

<img src="images/AddNewIngredientSequenceDiagram.png" width="963" />

Choose a reason for hiding this comment

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

Perhaps the sequence diagrams for various commands could be combined, instead of having separate diagrams for each command, such as with alt blocks.

References
1. Developer Guide: https://se-education.org/addressbook-level3/DeveloperGuide.html
2. User Guide: https://se-education.org/addressbook-level3/UserGuide.html
3.

Choose a reason for hiding this comment

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

Perhaps could remove point 3.


### Architecture

<img src="images/ArchitectureDiagram.png" width="280" />

Choose a reason for hiding this comment

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

Unable to see image of the architecture diagram

Copy link

@antrikshdhand antrikshdhand left a comment

Choose a reason for hiding this comment

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

Sequence diagrams throughout your DG all seem to follow accepted practice including activation bars for self-invocations, dotted lines for return values, and constructor invocations. Lovely diagrams!

Comment on lines +116 to +134
- **Step1**

`AddRecipeCommand` will parse the recipe title using `RecipeParser`. Then, it will return the recipe title.


- **Step2**

`AddRecipeCommand` will create a new `Recipe` with the obtained title.


- **Step3**

`AddRecipeCommand` will add newly created `Recipe` into `RecipeList`. Then, the recipe will be added into an
`ArrayList` inside `RecipeList`.


- **Step4**

`AddRecipeCommand` will call `Ui` class to print out the title of the recently added recipe.

Choose a reason for hiding this comment

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

Maybe for the addRecipeCommand, you can consider explaining how the title parsing and RecipeList work in more detail. Provide a step-by-step breakdown of the process, including the role of RecipeParser and RecipeList.

| v1.0 | beginner user | add new recipes into the list | learn and try out new recipes |
| v1.0 | beginner user | see all ingredient I have | |
| v1.0 | beginner user | add ingredients to the empty list | add an item to my kitchen inventory |
| v2.0a | amateur | delete some recipes | remove a recipe that I no longer want to use |

## Non-Functional Requirements

Choose a reason for hiding this comment

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

Maybe you can include a section or subsection on error handling to make the user understand. Explain how errors or exceptional cases are handled in the application. This can include user input validation, error messages, and how developers can handle exceptions in their code.

Copy link

@ShaniceTang ShaniceTang left a comment

Choose a reason for hiding this comment

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

Overall very nice and neat DG! Good job! 👍

Finally `Ui#showCommands()` will call `Ui#showRecipeCommands()`, `Ui#showIngredientCommands()`, `Ui#showOtherCommands()` to print all commands for recipe, ingredient and others respectively


<img src="images/HelpFunctionSequenceDiagram.png" width="732" />

Choose a reason for hiding this comment

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

Perhaps the command constructor could end here?
image

Comment on lines +64 to +81
- **Step 1**

Input will be sent from the main `EssenMakanan` class to the `Parser` to identify the command type


- **Step 2**

A new `HelpCommand` object will be created and will be sent back to main


- **Step 3**

`commandObject#executeCommand()` will be called which in turn calls `Ui#showCommands()`


- **Step 4**

Finally `Ui#showCommands()` will call `Ui#showRecipeCommands()`, `Ui#showIngredientCommands()`, `Ui#showOtherCommands()` to print all commands for recipe, ingredient and others respectively

Choose a reason for hiding this comment

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

I like how you described the command step by step 👍


The ***Architecture Diagram*** given above explains the high-level design of the App.

Given below is a quick overview of main components and how they interact with each other.

Choose a reason for hiding this comment

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

Perhaps a sequence diagram could show how all the components interact with each other?


### Architecture

<img src="images/ArchitectureDiagram.png" width="280" />

Choose a reason for hiding this comment

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

Perhaps the link doesn't work?
image

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.

10 participants