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-F10-2] ClubInvMgr #25

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

Conversation

quitejasper
Copy link

ClubInvMgr is a desktop CLI app for inventory management for CCA clubs, especially for fast typists who can accomplish tasks quickly by typing out commands.

Copy link

@heekit73098 heekit73098 left a comment

Choose a reason for hiding this comment

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

Generally the DG is easy to read and understand

The `UI` component
- Displays salutations, prompts for user input, error messages and results of queries.
- Reads in user inputs
- Depends on the `Messages` and `InvMgrException` classes in the `Common` component. It displays messages stored in the `Messages` class and displays an error message whenever `InvMgrException` is invoked.
Copy link

@heekit73098 heekit73098 Mar 31, 2022

Choose a reason for hiding this comment

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

Introducing this component out of the blue can be quite confusing, especially when InvMgrException is in the exceptions package instead of common

@@ -4,26 +4,167 @@

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}

## Design & implementation
## Design

Choose a reason for hiding this comment

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

Consider adding an overall class diagram and introduce the major components so that it would not very confusing below when components are named suddenly.

a `Command` class based on the user input.

### Command Component
![CommandClassDiagram](img/CommandClassDiagram.png)

Choose a reason for hiding this comment

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

Consider enlarging this diagram as the words are significantly smaller than the other diagrams

+ getRawUserInput(): String
}

InvMgr --> "1" Ui

Choose a reason for hiding this comment

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

Should this be a composition instead?

Comment on lines 64 to 67
![DescCommandSequenceDiagram](img/DescCommandSequenceDiagram.png)
The following diagram shows the sequence diagram for retrieving the description of an item.

For a user who is unaware of what an item is about, he/she can enter the command eg. `desc 2` command to extract the description for the second item in the inventory list. This command is interpreted by the `Parser` and a `DescCommand` is returned to `InvMgr`. `InvMgr` calls the execute command of `DescCommand` which retrieves the item's information from the `ItemList` and then outputs them into the `Ui` for the user to see.

Choose a reason for hiding this comment

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

image
In the UML diagram, the program calls writeData(ArrayList) at the end after the execution of the desc command. Not sure if this was intended as this was not explained the running of the desc command.

Comment on lines 160 to 166
| v1.0 | Manager | Add a new item to the inventory | Update my inventory |
| v1.0 | Manager | Remove an item from the inventory | Update my inventory |
| v1.0 | Stocktaker | list out all my items | View all my items at a glance |
| v1.0 | New user | List out all possible commands | I can familiarise myself with using the system |
| v1.0 | User who has not seen items physically | Get the description of a particular item | I can visualise the item better to know if it is what i need |
| v1.0 | As a frequent/first time user | Write to a file containing the entire inventory | Save my inventory data to a file |
| v1.0 | Stocktaker | Read from and load an inventory file data | To work on and view the data |

Choose a reason for hiding this comment

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

Perhaps you can update the user stories with those you have selected for V2.0?

Comment on lines 6 to 10

## Design & implementation
## Design

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Application Launch

Choose a reason for hiding this comment

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

Consider adding a content page or navigation bar with hyperlink to improve navigability and document formatting

Comment on lines 97 to 106
![ListCommandSequenceDiagram](img/ListCommandSequenceDiagram.png)

The following diagram shows the sequence diagram of the listing of items in `itemList`.

The user starts by typing a list command.

1. `InvMgr` calls `parse("list")` method in `Parser` class, which returns a ListCommand object.
2. `InvMgr` calls `execute(itemList, ui)` method in `ListCommand` object.
3. `ListCommand` loops through every `Item` in `itemList` and prints them line by line
and numbers them.

Choose a reason for hiding this comment

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

image
Consider moving the end of lifeline cross out of the object box and at the end of the line.

{Describe the value proposition: what problem does it solve?}
1. Centralised management of resources that ensures accurate and timely allocation of equipment to students
2. Increases the ease and efficiency of resource management
3. More organised

Choose a reason for hiding this comment

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

Consider elaborating on this value proposition as it is quite vague. Could possible extend to be similiar like point 1.

Copy link

@cczhouqi cczhouqi left a comment

Choose a reason for hiding this comment

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

There are some minor things that can be improved (as shown in others' comments also), but generally it looks quite good 👍🏻

a `Command` class based on the user input.

### Command Component
![CommandClassDiagram](img/CommandClassDiagram.png)

Choose a reason for hiding this comment

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

You may also consider changing the way you arrange the boxes (to not in one line) to make the words larger but keep the diagram in a reasonable size.

6. `AddCommand` will converse with `Ui` to show a message that the item has been added. In this case, the item to add will be printed as the name of the item, followed by " has been added!".

### Delete Command
![DeleteCommandSequenceDiagram](img/DeleteCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

Some of the diagrams are above the "The following diagram" line while others are below. You may consider keeping it consistent :D

Copy link

@yanjie1017 yanjie1017 left a comment

Choose a reason for hiding this comment

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

Looks good in overall. May require some minor changes and add in the remaining sections.


### List Command
![ListCommandSequenceDiagram](img/ListCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

Screenshot 2022-03-31 at 11 22 16
Should the lifeline of the object carry on if the class has been terminated?

### Parser Component

![ParserClassDiagram](img/ParserClassDiagram.png)

Choose a reason for hiding this comment

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

Screenshot 2022-03-31 at 11 27 54
Consider stating the return types of the methods called in InvMgr?

### Parser Component

![ParserClassDiagram](img/ParserClassDiagram.png)

Choose a reason for hiding this comment

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

The format of attributes should be
NameOfAttribute : DataTypeOfAttribute


### Description Command
![DescCommandSequenceDiagram](img/DescCommandSequenceDiagram.png)
The following diagram shows the sequence diagram for retrieving the description of an item.

Choose a reason for hiding this comment

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

Screenshot 2022-03-31 at 11 34 48
Screenshot 2022-03-31 at 11 34 58
Inconsistent format. Can decide whether to add the footer.

thernturnaround and others added 26 commits April 9, 2022 18:09
…alDate

BorrowStatus had no way of being updated before.

To keep the listob, listfb commands relevant to the current date,

Let's
- Write an adapter for BorrowRecord
- Update the BorrowStatus everytime Storage loads in data
Refactor CommandStubs to ItemList Stubs
Added equals() check for BorrowRecords, needed for equality check in Storage tests
This is to help resolve the JSON serializer issue when storing the data.
Java, by default, adds references of objects into arraylist. This means the original Items in ItemList may be modified.

This may result in a race condition! Each test are given their own threads to execute in JUnit.

To fix this, we should clone the Item and add them to the list before returning the itemlist.

Let's
- Implement clone() for BorrowRecord, needed for deep copying
- Implement a deep copy of Item() in its clone method to clone BorrowRecord
- Change ItemListStubs to generate new mutable ItemLists that are clones of the specified Items.
Update BorrowRecord and BorrowCommand
…into branch-general-bugfix

Adapted new code into BorrowRecordAdapter
Cloneable and clone is frowned upon.

Let's remove its usages and create manual copies through a copyItem()/copyBorrowRecord() instead.
Some weird, undiagnosable behaviour happened when the lists were globally defined in ItemListStubs.

Defining them locally (within the CommandTest) seems to resolve the issue
chihyingho and others added 30 commits April 11, 2022 23:16
Updates DG with ReturnCommand, LostCommand and HelpCommand implementation details
…into A-UserGuide

# Conflicts:
#	docs/UserGuide.md
…into A-DeveloperGuide

# Conflicts:
#	docs/DeveloperGuide.md
Update user stories for v2.1
Updates to PPP: lestersimjj
Update DeveloperGuide.md for Jasper
Update PPP link in AboutUs.md for IncompetentDev
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