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

[CS2113T-T09-1] Control Your Crowd #13

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

Conversation

iamakilahamed
Copy link

Safest Entry Tracker (SET) is a desktop app to track and access customer personal details and their profiles during pandemics via a Command Line Interface (CLI).


Classes used by multiple components are in the `seedu.Duke.commons` package.

## Implementation
Copy link

Choose a reason for hiding this comment

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

Perhaps there can be an implementation component to help developers understand how Safest Entry Tracker is implemented?

* a `Phone` object

### Storage component
**API** : [`seedu.duke.storage`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/storage) package
Copy link

Choose a reason for hiding this comment

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

this diagram looks quite good :)

@iamakilahamed iamakilahamed changed the title [CS2113T-T09-1] Safest Entry Tracker [CS2113T-T09-1] Control Your Crowd Mar 31, 2021
Copy link

@Cocokkkk Cocokkkk left a comment

Choose a reason for hiding this comment

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

Although this is not the final version, this DG looks well-formatted, easy to read, and navigate, including some visuals. I believe it would be better after your further improvement. Looking forward to your final version. :)


## Product scope
### Target user profile
### Architecture
Copy link

Choose a reason for hiding this comment

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

It's good to contain the architecture section. Maybe it can be better to provide a diagram to show the overall architecture. ( Refer to the AB3 DG example)


### Common classes

Classes used by multiple components are in the `seedu.Duke.commons` package.
Copy link

Choose a reason for hiding this comment

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

It's decent to have common classes to be used by multiple components. What's in this class? Are constants and messages or something else? Perhaps it would be better to give more details. :)

**API** : [`seedu.duke.person`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/person) package


<img src="images/ModelComponentStructure.png" width="600" height="600" />
Copy link

Choose a reason for hiding this comment

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

This digram looks quite good! :)


Classes used by multiple components are in the `seedu.Duke.commons` package.

## Implementation
Copy link

Choose a reason for hiding this comment

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

The implementation is quite important in DG. Perhaps your team hasn't started yet, it would be better to show every important feature here with the help of literal explanation and sequence diagrams! Looking forward to your results.

Copy link

@zikunz zikunz left a comment

Choose a reason for hiding this comment

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

The Design section of this Developer Guide is of very high quality and it is indeed great work! 👍

Almost no possible bugs in the aspects of UML diagrams, use of visuals, explanations, neatness / correctness are observed.

Keep up the great work! :)

The `UI` component,

* Reads user commands using `Scanner` class.
* Pass raw user commands into `Parser` class under **Logic Component**.
Copy link

Choose a reason for hiding this comment

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

You can perhaps use "passes" here for standardization purposes :)


Given below is the Sequence Diagram for interactions within the `Logic` component for the `parseCommand("clear)"` API call.

![](images/LogicComponentSequenceDiagram.png?raw=true "Logic component Sequence Diagram for clear")
Copy link

Choose a reason for hiding this comment

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

Is the "calling arrow" correct in illustraing the interactions between :ClearParser and :ClearCommand?

Capture


Given below is the Sequence Diagram for interactions within the `Logic` component for the `parseCommand("clear)"` API call.

![](images/LogicComponentSequenceDiagram.png?raw=true "Logic component Sequence Diagram for clear")
Copy link

Choose a reason for hiding this comment

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

Should there be a "return arrow" here?

Capture1


### Logic component

![](images/LogicComponentStructure.png?raw=true "Structure of Logic Component")
Copy link

Choose a reason for hiding this comment

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

A well designed diagram with correct notations!

For, "XYZParser = CheckInParser, FindParser, etc" it could look like assignment here. Perhaps you can make it free from any ambiguity :)

Capture3


### Model component

**API** : [`seedu.duke.person`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/person) package
Copy link

Choose a reason for hiding this comment

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

I like this diagram, it is both comprehensible and well integrated into the description!

* Reads user commands using `Scanner` class.
* Pass raw user commands into `Parser` class under **Logic Component**.
* After the **Logic Component** has executed the function, it will then return `CommandOutput` object
which contains the outcome of the execution, fail or succeed.
Copy link

Choose a reason for hiding this comment

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

Is this "failure or success"?



### Storage component
**API** : [`seedu.duke.storage`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/storage) package
Copy link

Choose a reason for hiding this comment

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

Great use of hyerlink here as well :)


### Storage component
**API** : [`seedu.duke.storage`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/storage) package
![](images/storage_module.png?raw=true "Storage Module diagram")
Copy link

Choose a reason for hiding this comment

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

This diagram is also great and the only one advice which could be given is regarding the size of this diagram. You could consider resizing it so that the text size in the diagram matches that of the main text. :)

Copy link

@rageqqq rageqqq left a 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 with only minor issues

Comment on lines +58 to +59
5. The result of the command execution is encapsulated as a `CommandOutput` object which is passed back to the `TextUi`.
6. In addition, the `CommandOutput` object can also instruct the `TextUi` to perform certain actions, such as displaying the list to the user.
Copy link

Choose a reason for hiding this comment

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

Not clearly shown in the logic diagram, shouldn't the arrow point to the UI class since the CommandOutput calls the TextUi?


## Product scope
### Target user profile
### Architecture
Copy link

Choose a reason for hiding this comment

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

An overall diagram that shows how each major component interacts with one another would be useful

Comment on lines +75 to +84
The Model component,

* contains a `Person` class which represents a person who checks in/out.
* contains a `TrackingList` class which uses an ArrayList to keep track of all the `Person` objects who have currently checked in/out.
* contains a `PersonLog` class which uses a HashMap to permanently store all the `Person` objects who have checked in before.

A `Person` object contains:
* an `Id` object
* a `Name` object
* a `Phone` object
Copy link

Choose a reason for hiding this comment

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

Location component is not explained

Comment on lines 19 to 161
|Version| As a ... | I want to ... | So that I can ...|
|Priority| As a ... | I want to ... | So that I can ...|
|--------|----------|---------------|------------------|
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|
|*****|mall staff|be able to review the personal particulars of the customer|be aware of who he is|
|*****|new user|be able to use the program without much training| use the program as soon as possible and easily implement it|
|****|mall staff|key in personal particulars easily into the application| check in visitors quickly without holding up a line of customers|
|****|mall staff|be able to easily key in a unique personal identifier| check in and find a visitor|
Copy link

Choose a reason for hiding this comment

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

I like how priorities are added to the user stories to show the importance. But it seems like more user stories can be added



### Storage component
**API** : [`seedu.duke.storage`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/storage) package
Copy link

Choose a reason for hiding this comment

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

Inconsistent formatting, Tracking List Encoder should be written as TrackingListEncoder


The `Storage` component,
* saves and encodes `Person` objects in `Tracking List` into a `.txt` file.
* reads a `.txt` file of valid encoding and populates a `TrackingList`

Choose a reason for hiding this comment

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

Consider defining the term "valid encoding"

Comment on lines 158 to 161
|*****|mall staff|be able to review the personal particulars of the customer|be aware of who he is|
|*****|new user|be able to use the program without much training| use the program as soon as possible and easily implement it|
|****|mall staff|key in personal particulars easily into the application| check in visitors quickly without holding up a line of customers|
|****|mall staff|be able to easily key in a unique personal identifier| check in and find a visitor|

Choose a reason for hiding this comment

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

Consider adding a legend section that explains the use of the asterisks


<img src="images/ModelComponentStructure.png" width="600" height="600" />

*Figure #. Structure of the Model Component*

Choose a reason for hiding this comment

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

Missing figure number


### Logic component

![](images/LogicComponentStructure.png?raw=true "Structure of Logic Component")

Choose a reason for hiding this comment

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

Consider standardizing size of diagrams

**API** : [`seedu.duke.person`](https://github.com/AY2021S2-CS2113T-T09-1/tp/tree/master/src/main/java/seedu/duke/person) package


<img src="images/ModelComponentStructure.png" width="600" height="600" />

Choose a reason for hiding this comment

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

Comprehensive and detailed diagram

Comment on lines +102 to +105
`TrackingListEncoder` takes the currently stored `TrackingList` and converts it to a more storage friendly String.
This String is then written to a file on the disk, known on the diagram as `StoredTrackingList`.
As the name suggests, `TrackingListDecoder` achieves the reverse by taking the file on drive, `StoredTrackingList`,
and interprets the file to be loaded into the memory as a `TrackingList`.

Choose a reason for hiding this comment

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

Clear explanation

Copy link

@vvvvh123 vvvvh123 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 the others have already done a good job reviewing your guys DG. I really like the diagrams that you used; I think they are very clear and easy to understand. You could consider adding an overall architecture section explaining how you've split up the components before diving in.

Comment on lines +47 to +48

![](images/LogicComponentStructure.png?raw=true "Structure of Logic Component")
Copy link

Choose a reason for hiding this comment

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

image
I really like the use of the notes to make it clear what XYZCommand and XYZParser is referring to

sarzorwyn and others added 30 commits April 12, 2021 22:03
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.