-
Notifications
You must be signed in to change notification settings - Fork 248
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-T14-2] BagPacker #61
base: master
Are you sure you want to change the base?
[CS2113-T14-2] BagPacker #61
Conversation
add authorship for Sherlock-YH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good explanation of some commands with UML diagram. It would be better to include more detail in the DG.
When `contains()` returns `true`, method `addItem(item)` will be interrupted and a `DuplicateItemException` will be thrown from `packingList`, which will be caught by `a`. If `contains()` returns false, the item will be added onto `packingList`. | ||
|
||
In both scenarios, `ui.printToUser` will be called to print a message to the user. In the former case, `ExistItemError` will be printed, while `AddSuccess` will be shown if the item was added with no issues. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good workflow explanation of the command
| v2a | user | find an item by name | find the pack status of an item without having to go through the entire list | | ||
| v2a | user | remove my packing list | clear my list once I am done packing | | ||
| v2a | user | Specify the quantity of an item I need to pack | keep track of individual item quantities being packed | | ||
| v2b | user | Save my packing list | keep track of my packing list even after leaving the app | | ||
|
||
## Non-Functional Requirements | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be better to add in instructions for manual testing to get an overview of all commands?
# Developer Guide | ||
|
||
## Acknowledgements | ||
|
||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the | ||
original source as well} | ||
|
||
## Design & implementation | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be better to mention the main components of the app architecture to understand the overview of components used?
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good use of UML diagram to understand workflow of the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing sequence diagrams are good enough, but the DG definitely lacks class diagrams.
Otherwise, the description gives enough information to understand the product and its implementation, though more diagrams would decrease mental effort needed.
docs/DeveloperGuide.md
Outdated
|
||
When using the `add` function, we have decided to implement a passive function that checks whether the item with the same name already exists in the packingList. | ||
|
||
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe class diagram would better clarify interactions between the classes rather than text description?
docs/img_1.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice decision to include the reference frame!
docs/img_1.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getDescription
calls get pretty confusing here - maybe try to create another activation bar for detaching them to clarify the functionality?
|
||
Delete command is used to delete an item from the packing list. | ||
|
||
Mechanism: ```DeleteCommand.execute()``` calls the ```PackingList.deleteItem()``` method from the ```PackingList``` class which executes the ```ArrayList.remove()``` method to remove the item from the ```PackingList``` ArrayList. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to see the diagrams here!
docs/img_1.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Exception thrown back
statement mean actual throw
or just showing the error to the user? Would be nice to have it more clearly stated or visualized!
|--------|----------|---------------|------------------| | ||
|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| | ||
| Version | As a ... | I want to ... | So that I can ... | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job with specifying the user stories by version! Gives a good understanding of the actual program and the progress made
docs/DeveloperGuide.md
Outdated
# Developer Guide | ||
|
||
## Acknowledgements | ||
|
||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the | ||
original source as well} | ||
|
||
## Design & implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more idea would be to create a detailed class diagram of the Exception classes - this would greatly help developers with what to use when handling new exceptions for new functionality
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) | ||
When `execute()` is called in `a`(object of class `AddCommand`), the `addItem(item)` method is called in the object `packingList`. This method will see if method `contains()` will return `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to give the AddCommand
instance a more descriptive name?
docs/DeveloperGuide.md
Outdated
The `contains()` method, which is shown as a reference on the right of the UML diagram, is a boolean method that loops through all items in the packingList, and returns `true` if any of the existing items have the same description as the item to be added in `toAdd`, and `false` otherwise. | ||
|
||
When `contains()` returns `true`, method `addItem(item)` will be interrupted and a `DuplicateItemException` will be thrown from `packingList`, which will be caught by `a`. If `contains()` returns false, the item will be added onto `packingList`. | ||
|
||
In both scenarios, `ui.printToUser` will be called to print a message to the user. In the former case, `ExistItemError` will be printed, while `AddSuccess` will be shown if the item was added with no issues. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to place this explanation in front of the diagram?
docs/DeveloperGuide.md
Outdated
#### Bye Command | ||
```ByeCommand``` is used to exit the BagPacker application. | ||
|
||
Mechanism: ```ByeCommand.execute()``` updates the static boolean ```isBagPackerRunning``` to be false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which class is the isBagPackerRunning
static variable stored? Would it be a good idea to specify?
|
||
Delete command is used to delete an item from the packing list. | ||
|
||
Mechanism: ```DeleteCommand.execute()``` calls the ```PackingList.deleteItem()``` method from the ```PackingList``` class which executes the ```ArrayList.remove()``` method to remove the item from the ```PackingList``` ArrayList. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and concise explanation of mechanism
2. Create command object - The ```Parser``` class creates a corresponding command object of the relevant command | ||
3. Execute command object - ```runBagPacker()``` method executes the ```.execute()``` method (overridden by child classes) of the command object | ||
which runs the actual command function | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to see some class and sequence diagrams
docs/DeveloperGuide.md
Outdated
| v2a | user | find an item by name | find the pack status of an item without having to go through the entire list | | ||
| v2a | user | remove my packing list | clear my list once I am done packing | | ||
| v2a | user | Specify the quantity of an item I need to pack | keep track of individual item quantities being packed | | ||
| v2b | user | Save my packing list | keep track of my packing list even after leaving the app | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very well-managed by specifying the version to be implemented and prioritizing the key features.
#### Help Command | ||
Help command is used to exit the BagPacker application. | ||
|
||
Execute: ```HelpCommand.execute()``` prints the following help message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to know what type of help commands are available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add detailed diagram to all the features
docs/DeveloperGuide.md
Outdated
@@ -1,29 +1,115 @@ | |||
[DeveloperGuide.md](DeveloperGuide.md) | |||
# Developer Guide | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to include table of contents in the top of the DG
docs/DeveloperGuide.md
Outdated
# Developer Guide | ||
|
||
## Acknowledgements | ||
|
||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the | ||
original source as well} | ||
|
||
## Design & implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to include Main architectural design and class diagram for each class to showcase the whole project design
docs/DeveloperGuide.md
Outdated
It then updates the ```quantity``` variable according to the quantity inputted by the user. | ||
|
||
|
||
#### Preventing duplicate items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preventing duplicate items
and add command
now are on the same layer which makes it ambiguous. Since preventing duplicate items
is under add command
then it should be one layer down
docs/DeveloperGuide.md
Outdated
|
||
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specifying the below diagram as Sequence Diagram would be a good idea
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Exception handling in Sequence Diagram is ambiguous.
- Missing self invocation activation bar for
.add(item)
- Suggest to check out the correct method to showcase static method.(It should point to activation bar)
- Incorrect use of Reference frame, should check out module website( should have ref and sd frame)
- Return of
true
andfalse
should be dashed line. - As it is already in loop frame, there is no need to do self-invocate the whole activation bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added my reviews! Good effort!
docs/DeveloperGuide.md
Outdated
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. | ||
![img_1.png](img_1.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
# Developer Guide | ||
|
||
## Acknowledgements | ||
|
||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the | ||
original source as well} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be better to give a brief overview of how your application works ] at the beginning of the docs, including the main commands and components, before diving into each component individually.
docs/DeveloperGuide.md
Outdated
|
||
This is done through the `contains()` method in class `PackingList()`, which is called during `execute` in an `AddCommand` object. | ||
|
||
Below is the UML diagram showing what occurs during `add` function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to add the return of control in your diagram.
|
||
## Design & implementation | ||
|
||
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
### Command Mechanisms: | ||
For all valid commands, the mechanism of implementation are as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if you could try standardising whether to add the full stop at the end of the sentence.
Add Mac prompts to UG
This reverts commit 961ae6c.
Edit code formatting and wording
Change error message
Create packall feature
Save and load feature added
fix - improve load() function in Storage class to read quantities of above 9
Update User Guide
Update PPP
Bug fixing
Add line breaks
Fix bug - characters get cut off in pdf
Refactor Linus' PPP
Add line break
Fix DG broken link and more
code block fix
handled test fix
fix io tests
BagPacker is an app to help travelers manage their packing lists.