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-4] SuperTracker #41

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

Conversation

rismm
Copy link

@rismm rismm commented Mar 7, 2024

SuperTracker helps to streamline the process of checking inventory for supermarket inventory managers.

JeffinsonDarmawan pushed a commit to JeffinsonDarmawan/tp that referenced this pull request Mar 21, 2024
…ugFixes

Fix bug for add and remove flower command
yeozongyao added a commit to yeozongyao/tp that referenced this pull request Mar 29, 2024
…yao-categoriseBooks

Categorise the books with personalised labels and with the genre
+Item(name:String, quantity:int,
price:double, expiryDate:LocalDate)
+getName():String
}

Choose a reason for hiding this comment

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

Should you explain why SuperTracker depend on FileManager?

Item <. FileManager
Item "*" <-* Inventory
FileManager <.. Command
Inventory <.. Command

Choose a reason for hiding this comment

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

Should you use standard docorator to represent the class type such as interface or basic class.

Ui --> ListCommand
deactivate Ui

end

Choose a reason for hiding this comment

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

In all your sequence diagram, only objects are interacting. So should you remove the <<class>> decorator above?

loop items

ListCommand -> Ui : listItem(item:Item, index:int, hasQuantity:boolean, hasPrice:boolean, hasExpiry:boolean, firstParam:String, secondParam:String):void)
activate Ui #e5c2ea

Choose a reason for hiding this comment

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

For the sequnce diagram, its too big, can you split it into small parts?

- `FileManager`: To save the new item added onto the hard disk

The following sequence diagram shows the execution of a NewCommand<br>
![NewCommandSequence](uml-diagrams/NewCommandSequence.png)

Choose a reason for hiding this comment

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

For the new Item(...), according to the module website, a constructor does not need a new keyword

Comment on lines 242 to 243
The following sequence diagram shows the execution of a ListCommand<br>
![ListCommandSequence](uml-diagrams/ListCommandSequence.png)

Choose a reason for hiding this comment

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

After calling a constructor, new ArrayList<>..., you can mark X in the end of the diagram to show that it is deleted

Copy link

@wallywallywally wallywallywally left a comment

Choose a reason for hiding this comment

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

Diagrams and explanations can be made more readable, but I like how thorough the design and implementation is!

Comment on lines 40 to 61
> Note:
> - All command regex are in the format:\
> `a/(?<grp1>.*) b/(?<grp2>.*) c/(?<grp3>.*) ` with `a/`, `b/`, `c/` being the respective parameter flags
> and `/` as the flag character.
> - As of now, parameter flags must only contain 1 character right before the `/`
> - Suppose we have `a/aaa b/bbb c/ccc ` as an input, according to the regex above,
> aaa will be in the named capture group "grp1", bbb in "grp2", ccc in "grp3"
> - If a parameter is optional, we would have it in a non-capture group with a `?` at the end
> - i.e. in `a/(?<grp1>.*) (?<grp2>(?:b/.*)?)`, the `b/` flag is an optional parameter
>
> For example, suppose the program wants to match the user input parameter
> `c/coconut a/apple b/bear a/anaconda d/donkey` to the regex `a/(?<grp1>.*) b/(?<grp2>.*) c/(?<grp3>.*) (?<grp4>(?:e/.*)?)`
> 1. The input parameter string and a string array of flags `{a, b, c, e}` is passed into `Parser-makeStringPattern(...)`
> 2. `Parser-makeStringPattern` will return a pattern string `a/apple b/bear c/coconut `
> 3. The program will then try to match the pattern string `a/apple b/bear c/coconut ` to the regex `
> a/(?<grp1>.*) b/(?<grp2>.*) c/(?<grp3>.*) (?<grp4>(?:e/.*)?)`
> 4. The above pattern string matches the regex, with the following strings in the named capture groups:
> - "grp1": apple
> - "grp2": bear
> - "grp3": coconut
> - "grp4" is empty

Choose a reason for hiding this comment

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

The explanation here feels repetitive, and can be shortened.

Comment on lines 94 to 102
#### Loading
Loading is performed at the start of the program in `SuperTracker-run()` where it calls the method `FileManager+loadData()`.
`loadData()` looks for the save text file at `./data/items.txt` and reads the lines from the file `items.txt` exists. As indicated earlier,
each line containing a single `Item`'s data will be in the format and order of `NAME,QUANTITY,PRICE,EXPIRY_DATE`. Each attribute will be parsed into its
relevant data type, creating a new `Item` with the extracted attributes, which is then added to the item list in `Inventory`. In the event where the line
of data read is not in the correct format, or the attributes are unable to be parsed into its relevant data type (i.e. the string in the QUANTITY part reads "f1ve" instead of "5")_,
the line will be ignored and a new `Item` will not be added to the list.

The parsing of string data to an `Item` object is handled by the method `FileManager-parseItemData()`.

Choose a reason for hiding this comment

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

This explanantion can be formatted in a way that is easier to read, perhaps using a list.

Saving and loading data of `Item` objects is performed by the `FileManager` class. As the program is running,
`Item` objects will be stored in the `Inventory` class. The following is a class diagram of `FileManager`
and its relevant dependencies\
![FileManagerClass](./uml-diagrams/FileManagerClass.png)

Choose a reason for hiding this comment

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

Class diagram is too complicated, can remove redundant attributes or methods to make it easier to understand for this implementation.


### New Command
The following is a class diagram of the NewCommand and its relevant dependencies<br>
![NewCommandClass](uml-diagrams/NewCommandClass.png)

Choose a reason for hiding this comment

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

Sequence diagram is too complicated. The new Item(...) call is too long and can be simplified.

- `Ui`: To notify the user about the successful execution of `DeleteCommand`

The following sequence diagram shows the execution of a DeleteCommand<br>
![DeleteCommandSequence](uml-diagrams/DeleteCommandSequence.png)

Choose a reason for hiding this comment

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

Activation bar for Inventory is not consistent. contains() is green while delete() is white.

The following sequence diagram shows the execution of a NewCommand<br>
![NewCommandSequence](uml-diagrams/NewCommandSequence.png)

1. The `SuperTracker` class calls the `execute` method of `NewCommand`

Choose a reason for hiding this comment

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

SuperTracker is not explained enough previously to let the reader understand it is responsible for a lot of the main functionality.


### List Command
The following is a class diagram of the FindCommand and its relevant dependencies<br>
![ListCommandClass](uml-diagrams/ListCommandClass.png)

Choose a reason for hiding this comment

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

Class diagram is too complicated. I think it can be broken down into smaller parts in other diagrams and displayed when those parts are being explained.

- `Inventory`: For getting the list of items in the inventory
- `Ui`: To print the list of items in the inventory to the output

The 7 parameters in the constructor `hasQuantity`, `hasPrice`, `hasExpiry`, `firstParam`, `secondParam`, `sortBy`, `isReverse` are used to determine how the list should be printed out.

Choose a reason for hiding this comment

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

Explanation is too long and can be presented in a more readable way, perhaps a table.

Comment on lines 239 to 247
There are 8 sorting modes in total
1. `sortBy == ""` and `isReverse == false`: Alphabetical ascending (e.g. A-Z)
2. `sortBy == ""` and `isReverse == true`: Alphabetical descending (e.g. Z-A)
3. `sortBy == "q"` and `isReverse == false`: Quantity ascending
4. `sortBy == "q"` and `isReverse == true`: Quantity descending
5. `sortBy == "p"` and `isReverse == false`: Price ascending
6. `sortBy == "p"` and `isReverse == true`: Price descending
7. `sortBy == "e"` and `isReverse == false`: Expiry date ascending (e.g. 2024-2025)
8. `sortBy == "e"` and `isReverse == true`: Expiry date descending (e.g. 2025-2024)

Choose a reason for hiding this comment

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

Explanation is not very easy, can use a table perhaps.

8. `sortBy == "e"` and `isReverse == true`: Expiry date descending (e.g. 2025-2024)

The following sequence diagram shows the execution of a ListCommand<br>
![ListCommandSequence](uml-diagrams/ListCommandSequence.png)

Choose a reason for hiding this comment

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

This sequence diagram is way too large and complicated.

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Input Parsing
The program handles user inputs in the Parser class where inputs are parsed into command classes that implement the Command interface.
The Parser class only contains static methods as we have determined that it would be more practical instead of instantiating an instance of a Parser class.
Copy link

Choose a reason for hiding this comment

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

Do you mind explaining why it would be more practical?

Comment on lines 76 to 77
Saving is performed automatically each time the item list is updated. Currently, there are only 5 commands that can make changes to
the item list and will call `FileManager+saveData()` at the end of `Command+execute()`.
Copy link

Choose a reason for hiding this comment

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

Will the entire save file be overwritten each time one of the commands are ran, or only the part that is being changed?


#### Loading
Loading is performed at the start of the program in `SuperTracker-run()` where it calls the method `FileManager+loadData()`.
`loadData()` looks for the save text file at `./data/items.txt` and reads the lines from the file `items.txt` exists. As indicated earlier,
Copy link

@hwc0419 hwc0419 Apr 5, 2024

Choose a reason for hiding this comment

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

What happens if the directory 'data' or the file 'items.txt' doesn't exist?

> - All command regex are in the format:\
> `a/(?<grp1>.*) b/(?<grp2>.*) c/(?<grp3>.*) ` with `a/`, `b/`, `c/` being the respective parameter flags
> and `/` as the flag character.
> - As of now, parameter flags must only contain 1 character right before the `/`
Copy link

Choose a reason for hiding this comment

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

Maybe it would be good to handle the error if the user enters more than 1 character, or edit the regex pattern string to specify that there must be only one character

rismm and others added 30 commits April 15, 2024 19:38
Change example for clear command
Fix class diagrams for exp and rev commands
Fix delete command class diagram
Update report command sequence diagram
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.

9 participants