Skip to content

[CS2113-W12-3] MagusStock#39

Open
ngkaiwen123 wants to merge 707 commits into
nus-cs2113-AY2223S2:masterfrom
AY2223S2-CS2113-W12-3:master
Open

[CS2113-W12-3] MagusStock#39
ngkaiwen123 wants to merge 707 commits into
nus-cs2113-AY2223S2:masterfrom
AY2223S2-CS2113-W12-3:master

Conversation

@ngkaiwen123

@ngkaiwen123 ngkaiwen123 commented Mar 2, 2023

Copy link
Copy Markdown

MagusStock is an inventory management system (IMS) that makes use of the command line interface for businesses to manage their stock inventory.

vishnuvk47 pushed a commit to vishnuvk47/tp that referenced this pull request Mar 15, 2023

@tsx0314 tsx0314 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall good job! Everything is clear and detailed! However, many explanations are used in the picture, sometimes it is hard to read

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The diagram is a bit hard to read. Perhaps you can omit some of the details

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The square may be removed it should be "+" and "-"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great! it is clear and easy to read!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

need to replace the square with -

@douph810975 douph810975 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In general, the diagrams are clear and comprehensive.

Comment thread docs/SequenceDiagram.png

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A clear diagram here! I have a small question, why after the user calls the run function in MagusStock the MagusStock still call the run function to itself?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This architecture diagram is clear and easy to understand!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The picture is unable to display here. Are there any bugs here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A meticulous diagram! I have a small problem, why after the updateItemQuality funciton, a new activation bar is created?

@chungnicholas chungnicholas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Developer Guide is well done and formatting is neat. Good job!

Comment thread docs/DeveloperGuide.md
## Acknowledgements

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
The documentation and organisation of our project follows the recommended format as shown in [SE-Education](http://se-education.org/addressbook-level3/DeveloperGuide.html)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider having a full stop at the end of the sentence

Comment thread docs/DeveloperGuide.md

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### 1.1. Architecture Design Diagram
![Architecture Diagram](ArchitectureDiagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice architecture design diagram

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +53 to +72
### 2.1. List
The list command is mainly handled by the `ListCommand` class, which extends the `Command` class.

![ListCommand.png](UML%2FList%2FListCommand.png)


**Step 1**. When the user executes the command `list`, the `ParserHandler` will create a new `ListParser` object and pass to it the `Inventory` where the items to be listed are stored.

![ListStep1.png](UML%2FList%2FListStep1.png)

**Step 2**. The `run` method in `ListParser` overrides the `run` method in `Parser` to create a new `ListCommand` object, passing to it the relevant `Inventory`.

![ListStep2.png](UML%2FList%2FListStep2.png)

**Step 3**. The `run` method in `ListCommand` overrides the `run` method in `Command` and calls the `listItems` method. The `listItems` method checks if the inventory is empty. If the inventory is empty, the method prints a message to inform the user that there are no items in the inventory. Otherwise, the `printTable` method from the `Ui` object is called.

![ListStep3.png](UML%2FList%2FListStep3.png)

**Step 4.**. If the `printTable` method is called, it takes in an `ArrayList<Item> items` aas a parameter and prints out a table showing the name, UPC, quantity and price of all items in the inventory.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Step by step guide is very clear and accompanied by diagrams for better visualization. Very nice!

Comment thread docs/DeveloperGuide.md
Comment on lines +2 to 22
## Contents
- [Acknowledgements](#acknowledgements)
- [Design & Implementation](#design--implementation)
- [Implementation](#implementation)
- [List](#list)
- [Add](#add)
- [Edit](#edit)
- [Remove](#remove)
- [Search](#search)
- [Filter](#filter)
- [Alert](#alert)
- [History](#history)
- [Category](#category)
- [Product Scope](#product-scope)
- [Target User Profile](#target-user-profile)
- [Value Proposition](#value-proposition)
- [User Stories](#user-stories)
- [Non-Functional Requirements](#non-functional-requirements)
- [Glossary](#glossary)
- [Instructions for Manual Testing](#instructions-for-manual-testing)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Neat table of content. Maybe could consider adding numbering here as well, corresponding to those used in the headers so the reader can navigate easier when scrolling.

For example, this could be 2.3. Edit
image
corresponding to this
image

@lil1n lil1n left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sequence diagrams are very detailed, but there are some areas that can be improved, great effort overall

Comment thread docs/DeveloperGuide.md Outdated
specific pattern. If the input doesn't match the pattern, it prints an error message and returns. If the input matches
the pattern, it creates a new `Item` object using the parsed parameters and creates a new `AddCommand` object using the
`Inventory` object and `Item` object. It then calls the `run()` method of the `AddCommand` object to execute the add command.
![AddParser.png](UML%2FAdd%2FAddParser.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that there is an additional return arrow between AddParser and AddCommand?

Comment thread docs/DeveloperGuide.md Outdated
specific pattern. If the input doesn't match the pattern, it prints an error message and returns. If the input matches
the pattern, it creates a new `Item` object using the parsed parameters and creates a new `AddCommand` object using the
`Inventory` object and `Item` object. It then calls the `run()` method of the `AddCommand` object to execute the add command.
![AddParser.png](UML%2FAdd%2FAddParser.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if it is intended, but activation bar for AddParser seems to not be deactivated

Comment thread docs/DeveloperGuide.md Outdated
If the `SessionManager`'s `autoSave` flag is enabled, it writes the current inventory to a file using the `SessionManager`.

The `run()` method of the `AddCommand` class calls the `addItem()` method to execute the add command.
![AddCommand.png](UML%2FAdd%2FAddCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if it is intended, but I think that there is a missing activaction bar when AddCommand calls addItems() and also missing return arrow from addItem()?

Comment thread docs/DeveloperGuide.md Outdated
If the `SessionManager`'s `autoSave` flag is enabled, it writes the current inventory to a file using the `SessionManager`.

The `run()` method of the `AddCommand` class calls the `addItem()` method to execute the add command.
![AddCommand.png](UML%2FAdd%2FAddCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if it is intended, but activation bar for AddCommand seems to not be deactivated

Comment thread docs/DeveloperGuide.md Outdated
If the `SessionManager`'s `autoSave` flag is enabled, it writes the current inventory to a file using the `SessionManager`.

The `run()` method of the `AddCommand` class calls the `addItem()` method to execute the add command.
![AddCommand.png](UML%2FAdd%2FAddCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if add(itemName, item), writeSession(inventory), writeCSV(inventory) is correct, but I think that method calls should be methodName(parameterName : parameterType)?

Comment thread docs/DeveloperGuide.md

Included below is a sequence diagram for the `RestockParser` and `RestockCommand` respectively:

![RestockParser.png](UML/Restock/RestockParser.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if it is intended, but diagram for :RestockParser, :RestockCommand, :SellParser and :SellCommand seems to be inconsistent with the rest of the diagrams as other diagrams do not have a : before the XYZParser and XYZCommand

Comment thread docs/DeveloperGuide.md
Included below is a sequence diagram for the `SellParser` and the `SellCommand` respectively:

![SellParser.png](UML/Sell/SellParser.png)
![SellCommand.png](UML/Sell/SellCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

there seems to be a lot of self calls, maybe can abstract out?

Comment thread docs/DeveloperGuide.md

### 1.2. UML Sequence Diagram

![Sequence Diagram](SequenceDiagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Classes have a missing : at the start. For example, :MagusStock.

Activation bar for MagusStock is also missing a closing line.
image

Comment thread docs/DeveloperGuide.md
### 2.1. List
The list command is mainly handled by the `ListCommand` class, which extends the `Command` class.

![ListCommand.png](UML%2FList%2FListCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The alt frame is missing an else path.

Comment thread docs/DeveloperGuide.md
### 2.1. List
The list command is mainly handled by the `ListCommand` class, which extends the `Command` class.

![ListCommand.png](UML%2FList%2FListCommand.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing separate sequence diagram to show details of ref frames.

Comment thread docs/DeveloperGuide.md
Comment on lines +59 to +72
**Step 1**. When the user executes the command `list`, the `ParserHandler` will create a new `ListParser` object and pass to it the `Inventory` where the items to be listed are stored.

![ListStep1.png](UML%2FList%2FListStep1.png)

**Step 2**. The `run` method in `ListParser` overrides the `run` method in `Parser` to create a new `ListCommand` object, passing to it the relevant `Inventory`.

![ListStep2.png](UML%2FList%2FListStep2.png)

**Step 3**. The `run` method in `ListCommand` overrides the `run` method in `Command` and calls the `listItems` method. The `listItems` method checks if the inventory is empty. If the inventory is empty, the method prints a message to inform the user that there are no items in the inventory. Otherwise, the `printTable` method from the `Ui` object is called.

![ListStep3.png](UML%2FList%2FListStep3.png)

**Step 4.**. If the `printTable` method is called, it takes in an `ArrayList<Item> items` aas a parameter and prints out a table showing the name, UPC, quantity and price of all items in the inventory.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use of non-standard visibility notation.

@HiIAmTzeKean HiIAmTzeKean left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall it is a great DG!

Comment thread docs/DeveloperGuide.md

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### 1.1. Architecture Design Diagram
![Architecture Diagram](ArchitectureDiagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clear architecture design

Comment thread docs/DeveloperGuide.md
Comment on lines +3 to 22
- [Acknowledgements](#acknowledgements)
- [Design & Implementation](#design--implementation)
- [Implementation](#implementation)
- [List](#list)
- [Add](#add)
- [Edit](#edit)
- [Remove](#remove)
- [Search](#search)
- [Filter](#filter)
- [Alert](#alert)
- [History](#history)
- [Category](#category)
- [Product Scope](#product-scope)
- [Target User Profile](#target-user-profile)
- [Value Proposition](#value-proposition)
- [User Stories](#user-stories)
- [Non-Functional Requirements](#non-functional-requirements)
- [Glossary](#glossary)
- [Instructions for Manual Testing](#instructions-for-manual-testing)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean and consise.

Comment thread docs/DeveloperGuide.md
Comment on lines +34 to +44
`MagusStock`: This is the entry point for the application, and it's responsible for starting the application and coordinating the interactions between the other components.

`ParserHandler`: This component is responsible for handling user input and determining which parser to execute based on the input. It uses a Parser to parse the input and generate a corresponding Command object.

`Parser`: This component is responsible for parsing the user input and generating a Command object. The ParserHandler uses the Parser to parse the input and determine which Command object to create.

`SessionManager`: This component is responsible for managing the inventory data persistence. It's connected to the Storage component, which reads and writes inventory data to and from a CSV file.

`Storage`: This component is responsible for reading and writing inventory data to a CSV file. It's connected to the SessionManager, which uses it to manage the inventory data persistence.

`Ui`: This component is responsible for displaying information to the user. It receives messages and data from the other components and displays them to the user.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great short summary of each component

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +97 to +106
#### 2.2.2. AddCommand Class
The `AddCommand` class is a command class that extends the `Command` abstract class. It represents the command to add an
item to the inventory. It contains a constructor that takes in an `Inventory` object and an `Item` object as parameters.
The constructor sets the `Inventory` object to be the inventory of the `Command` class, and the `Item` object to be the item
to be added to the inventory. The class also contains a private method called `addItem()` that adds the `Item` object to
the inventory. The `addItem()` method checks if the item already exists in the inventory using its unique UPC
(Universal Product Code) code. If the item already exists, it prints a message stating that the item is a duplicate.
Otherwise, it adds the item to the inventory, updates the item name hash, and adds the item to the UPC code history.
If the `SessionManager`'s `autoSave` flag is enabled, it writes the current inventory to a file using the `SessionManager`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like the way the classes are elaborated in segments so that developers can search a certain segment they want to read more about.

ysl-28 and others added 30 commits April 10, 2023 17:19
Update EditCommand for duplicate printing of category bug
add add and alert sample formats
DG: Update diagrams and text
Sync db storage status with actual status
Fix checkstyle and add javadoc
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