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-T13-2] LOTS #34

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

Conversation

andrewtkh1
Copy link

Large Order Tracking System (LOTS) is a Command Line (CLI) program that enables users to keep track of multiple food orders from a pre-set list of food items from different stores. The program helps users to collate the orders and displays a summary of all the orders along with other information such as each person’s order and special requirements (if any), cost of an individual’s order, total cost of all the orders and more. As LOTS is a CLI program, this would greatly benefit any user that excels in typing.

Copy link

@brendanlsz brendanlsz left a comment

Choose a reason for hiding this comment

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

Overall, I think your DeveloperGuide is very readable and informative. Just some small formatting issues to take care of. 👍

docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
Copy link

@astralum astralum left a comment

Choose a reason for hiding this comment

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

Good job overall! There are multiple minor grammatical errors, but they do not affect the overall readability of the DG.

@@ -1,38 +1,375 @@
# Developer Guide

Choose a reason for hiding this comment

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

Just wondering, why does your team have a DG for each member in your PR? It makes it confusing to see which DG is the one that's published.


## Design

### Logical Component

Choose a reason for hiding this comment

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

This component is named "Logic" and not "Logical" under "Main components of architecture". Maybe it would be better to maintain consistency of naming.


### Logical Component

The logical component of the program consists multiple classes. Namely: `Parser`,`Command` &

Choose a reason for hiding this comment

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

very small typo: missing whitespace between "Parser," and "Command"

are related to one another.
<br>![Logical Component Partial Class Diagram](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/LogicalComponentDiagrams/Logical%20Component%20Diagram-Page-2.jpg)

:information_source: **Note:** Specific command names are represented using a placeholder `'Abc'`, i.e. AddCommand, FindCommand.

Choose a reason for hiding this comment

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

Nice informational note!

Below is a step by step example on how the `Parser` interacts when a user keys in an input.

Step 1)<br>
Lets assume the user input is `delete 1/2`. `Duke` will then call the method

Choose a reason for hiding this comment

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

minor typo in "Lets", which should be "Let's"

The image below shows the codes for the switch statement.

![Switch case Code](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/ParserDiagrams/SwitchClassCode.jpg)

Choose a reason for hiding this comment

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

If the opt block results in an UnknownCommand being created, and so does the default case in the switch block, is it necessary to have the opt block?

Step 1)<br>
Lets assume the user input is `delete 1/2`. `Duke` will then call the method
`Parser.getCommand("delete 1/2")`. The method would split the user's input into an array of
strings along the spaces and store them in the array `listOfInputs`. After which,

Choose a reason for hiding this comment

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

"split by whitespaces/spaces" might be a better phrase than "along the spaces"

docs/DeveloperGuide.md Outdated Show resolved Hide resolved

Step 3)<br>
Next, `Duke` will then use the returned command to call `Command.execute()` which will interact
with the `Manager Components` of the program who will then remove the 2nd order from the first

Choose a reason for hiding this comment

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

Did you mean to write "Manager component" instead of "Manager Components"?
Also, you could split this sentence to avoid having two "which"s in one sentence.

break;
```
The upside of doing would be that there is less code overall.
However, doing so would cause our code to have a higher amount of coupling and would also

Choose a reason for hiding this comment

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

Nice explanation!


## Architecture

<br>![Architecture Diagram](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/ArchitectureDiagrams/ArchitectureDiagram.drawio.png)

Choose a reason for hiding this comment

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

Would be good if you are able to have consistent naming throughout the diagrams (eg Logic or Logical) and also use a standardised software to draw the images.


This is a sequence diagram of the `Parser` class.

![Squence Diagram of Parser](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/ParserDiagrams/Parser%20Sequence%20Diag.jpg)

Choose a reason for hiding this comment

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

The sequence diagram for a switch case should be quite clear and shouldn't be a need to include it here. Perhaps also can include the colours that are indicated in architecture make these diagrams even clearer

Comment on lines +174 to +177
<br>![Sequence Diagram](https://raw.githubusercontent.com/thaddyyz/tp/master/UMLdiagrams/EditCommandDiagrams/AddCommandSeqDiagram.png)

The Sequence Diagram below represents the interactions between components when user inputs command `delete 1/2`
<br>![Sequence Diagram 2](https://raw.githubusercontent.com/thaddyyz/tp/master/UMLdiagrams/EditCommandDiagrams/deleteCommandSeqDiagram.png)

Choose a reason for hiding this comment

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

the sequence diagrams here are a little too blurry :( the self invocation methods cannot be clearly seen too. Perhaps might want to reconsider using better diagrams and maybe not necessary to put both add and delete (since the sequence diagram for both are quite similar)


Hence, the sequence of which how Menu Command class and Order Command class are very similar. To prevent repeating of Sequence diagrams, a shared diagram will be listed below for the Menu Command Class and the Order Command class.

<br>![MenuAndOrdersSequenceDiagram](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/MenuAndOrdersSequenceDiagram/MenuAndOrdersSequenceDiagram.jpg)

Choose a reason for hiding this comment

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

this diagram is big and clear :) i think that one issue may be that the Ui.printOrdersList() should be pointing to the top of the activation bar instead of the middle of the activation bar.

if there is no match)

The following sequence diagram depicts how the `Logical` components interact with one another upon receiving the user's input of `"delete 1/2"`.
<br>![Logical Component Partial Sequence Diagram](https://raw.githubusercontent.com/AY2122S1-CS2113-T13-2/tp/master/UMLdiagrams/DeleteCommandDiagram/DeleteCommand%20Sequence%20Diagram.jpg)

Choose a reason for hiding this comment

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

This diagram looks very neat and clear, perhaps can use this as an example for your other diagrams (colour codes, size, etc)

thaddyyz and others added 25 commits November 1, 2021 13:21
Edited some diagrams of the logical component
Integrated Setup for dev readme into Dev guide.

Edited UML diagram to add background color.
WaiKit-nus and others added 30 commits November 6, 2021 22:26
Made some minor changes to DG and UG.
Add Header comment for AddCommandTest
Add Javadocs for test case. Removed img.png file which was not used. …
Added JavaDocs for deleteCommandTest
Added Value Proposition & Target User.
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