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-W12-3] MagusStock #39

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

Conversation

ngkaiwen123
Copy link

@ngkaiwen123 ngkaiwen123 commented Mar 2, 2023

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
Copy link

@tsx0314 tsx0314 left a comment

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

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

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

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

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 -

Copy link

@douph810975 douph810975 left a comment

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.

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?

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!

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?

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?

Copy link

@chungnicholas chungnicholas left a comment

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!


## 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)

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


{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)

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 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.

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 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)

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

Copy link

@lil1n lil1n left a comment

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

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

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?

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

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

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

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()?

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

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

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

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)?


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

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

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

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

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?


### 1.2. UML Sequence Diagram

![Sequence Diagram](SequenceDiagram.png)

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

### 2.1. List
The list command is mainly handled by the `ListCommand` class, which extends the `Command` class.

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

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.

### 2.1. List
The list command is mainly handled by the `ListCommand` class, which extends the `Command` class.

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

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 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.

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.

Copy link

@HiIAmTzeKean HiIAmTzeKean left a comment

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!


{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)

Choose a reason for hiding this comment

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

Clear architecture design

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)

Choose a reason for hiding this comment

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

Clean and consise.

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.

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 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`.

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