-
Notifications
You must be signed in to change notification settings - Fork 263
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
[CS2103T-F11-1] Calgo #25
base: master
Are you sure you want to change the base?
[CS2103T-F11-1] Calgo #25
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.
Keep up the good work!
image::ArchitectureSequenceDiagram.png[] | ||
|
||
The sections below give more details of each component. | ||
|
||
<<< | ||
|
||
[[Design-Ui]] | ||
=== UI component | ||
|
||
.Structure of the UI Component | ||
image::UiClassDiagram.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.
The arrow heads pointing to {abstract} UiPart are overlapping. It might be better to combine them?
docs/DeveloperGuide.adoc
Outdated
*Extensions* | ||
|
||
[none] | ||
*The `FoodRecord` is empty* + |
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.
Is the format of the extensions correct?
=== Use case: `find` `Food` item by keyword (which can be an incomplete word) | ||
|
||
*MSS* | ||
|
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 use cases be this short? (only 2 steps)
Use case ends. | ||
|
||
[discrete] | ||
=== Use case: `delete` an existing `Food` item in current `FoodRecord` |
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 code blocks be used in the use case?
docs/DeveloperGuide.adoc
Outdated
|
||
NOTE: The lifeline for `UndoCommand` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram. | ||
.Activity Diagram for List command | ||
image::ListActivityDiagram.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.
Is an activity diagram appropriate for a trivial process?
docs/DeveloperGuide.adoc
Outdated
|
||
[appendix] | ||
== Glossary | ||
|
||
[[food]] Food:: |
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.
Is it necessary to explain what a food is in the glossary? It might be better to explain what the Food class encompasses in one of the above sections.
image::DeleteSequenceDiagram.png[] | ||
|
||
NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram. | ||
|
||
<<< | ||
|
||
[[Design-Model]] | ||
=== Model component | ||
|
||
.Structure of the Model Component | ||
image::ModelClassDiagram.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.
Should the arrows on the diagram be curved?
docs/DeveloperGuide.adoc
Outdated
|
||
image::UndoRedoState0.png[] | ||
image::ListCommandSequenceDiagram.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.
As the diagram is large, maybe you can consider putting updateFilteredFoodRecord() as a method which has a return arrow?
docs/DeveloperGuide.adoc
Outdated
The `nom` command adds a `Food` item consumed by the user into the `stomach`. The following activity diagram summarizes what happens when the user executes a `nom` command. | ||
|
||
.Activity Diagram for Nom | ||
image::NomActivityDiagram.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.
The "User executes Nom command" box has already stated that it's a Nom command before the branch conditional "[command is Nom]".
docs/DeveloperGuide.adoc
Outdated
Step 2. The user executes `delete 5` command to delete the 5th person in the address book. The `delete` command calls `Model#commitAddressBook()`, causing the modified state of the address book after the `delete 5` command executes to be saved in the `addressBookStateList`, and the `currentStatePointer` is shifted to the newly inserted address book state. | ||
Step 1: `UI` detects that the user has entered a `String`. + | ||
Step 2: `UI` passes this `String` into `Logic` to eventually produce a `ListCommand`. + | ||
Step 3: `Logic` executes the `ListCommand` to filter out relevant `Food` entries from the `Model`. + |
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 Step 3, does the ListCommand
filter out Food
entries?
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 really nice work guys, I like the initiative your team took to stream line user experience by using update instead of add and edit. Good work! 🥇
|
||
.Interactions Inside the Logic Component for the `delete 1` Command | ||
.Interactions Inside the Logic Component for the `delete n/Apple` Command |
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 think there is still remnants of AddressBook in this diagram where you guys mentioned person?
docs/DeveloperGuide.adoc
Outdated
[NOTE] | ||
If a command fails its execution, it will not call `Model#commitAddressBook()`, so the address book state will not be saved into the `addressBookStateList`. | ||
* **Alternative 1 (current choice):** Sort whenever `Food` is added or edited in the `Model`. | ||
** Pros: Will guarantee correctness of sorting. Furthermore, since `Food` objects are sorted whenever they are added or edited one by one, each sorting operation will not be too expensive. |
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 don't think the cost of sorting works that way, but I'm not maybe I'm not too certain what you mean by "sorting operation will not be too expensive". But it is true that you can insert an element into the sorted array in O(n) time, but not by insert then sort :D
Edits to UG for delete and update and suggestion feature
Added additional information
Help Window GUI fix, update DG
Improve DG consistency for lexicographical order.
… into AddressBook-to-Functional-Calgo # Conflicts: # docs/DeveloperGuide.adoc
…, and latest date.
Minor bug fixing
update Help Command with new command formats
update with minor PPP edits
UG DG vet through and merge
# Conflicts: # docs/UserGuide.adoc
Updated PPP
update PPP
remove some deadlinks
@buddhavineeth @jeremylow97 @eugenetyc @J-Dan23