Skip to content

[CS2113-T17-3] Stocker#15

Open
TeoHaoZhi wants to merge 517 commits into
nus-cs2113-AY2324S1:masterfrom
AY2324S1-CS2113-T17-3:master
Open

[CS2113-T17-3] Stocker#15
TeoHaoZhi wants to merge 517 commits into
nus-cs2113-AY2324S1:masterfrom
AY2324S1-CS2113-T17-3:master

Conversation

@TeoHaoZhi

Copy link
Copy Markdown

Stocker is a desktop app that will provide quick access to currently available stock, track incoming stock and expiration dates, and categorize drugs based on different labels. It is optimized for use via a Command Line Interface (CLI). If you can type fast, Stocker can get your inventory management tasks done faster than traditional GUI apps.

Brian030601 added a commit to Brian030601/tp that referenced this pull request Oct 18, 2023

@YongbinWang YongbinWang 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 a very good job. There is no major code quality issues that I can spot, code looks well abstracted and structured. Do take note of making sure your naming conventions are clear and similar throughout the project.

Do let me know if you need clarifications or disagree with my comments.

users = new HashMap<>();
loginStatus = false;
this.in = new Scanner(System.in);
interactor = new Ui();

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 using a more straightforward name for your Ui object.

Comment on lines +52 to +55
if (choice.equals("register")) {
return "register";
} else if (choice.equals("login")) {
return "login";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can consider making your checks not case sensitive if the users types "Register" or "Login", up to your team's discretion.

interactor.showPasswordMessage();
String password = in.nextLine();

while (password.equals("")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can consider creating a constant value for empty string.
static final String EMPTY_STRING = "";

Comment on lines +197 to +199
while ((line = reader.readLine()) != null) {
String[] parts = line.split(":", 2);
if (parts.length >= 2) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid use of magic literals such as ":".

* @throws IOException if file is not found
*/
public void loadFileContents(String filePath) throws IOException, InvalidDrugFormatException {
File holder = new File("./drugs.txt");

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 avoiding magic literals here.

return new CommandResult<>(String.format(MESSAGE_INVALID_COMMAND_FORMAT, MESSAGE_USAGE));
}

List<StockEntry> entries = inventory.getStockEntries();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe allEntries would be a better name.

/**
* Usage message for the 'find' command.
*/
public static final String MESSAGE_USAGE = COMMAND_WORD + " /n" + ": Finds drug in inventory using name." +

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unusually long constant. You can consider abstracting out some parts into smaller constants then combining them again,

public class Drug {

public String name;
String expiryDate;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there supposed to be an access modifier here?

Comment on lines +56 to +57
public String toString() {
return "Name: " + name + ", Expiry Date: " + expiryDate;

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 avoiding magic literals here.

Comment on lines +30 to +32
assert inventory != null : "Inventory should be initialised before executing ListCommand.";
// Retrieve the list of drugs from the inventory
List<StockEntry> stockEntries = inventory.getStockEntries();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"stockEntries" variable name different from FindCommand class. There should be similar naming convention across the project.

Comment thread docs/DeveloperGuide.md Outdated
| \* \* \* | v1.0 | Receptionist | View a list of products of that category | Easily obtain an overview of the products |
| \* \* \* | v1.0 | First-time user | See a list of all available actions | Better understand how to use the application |
| \* \* \* | v1.0 | Inventory Manager | Find a specific drug currently in the system | Check up its details and quantities specifically |
| \* \* \* | v2.0 | user | Find a to-do item by name | Locate a to-do without having to go through the entire list |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a small consideration to organise based on v1.0 then v2.0 so it looks more organised

Comment thread docs/DeveloperGuide.md

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
1. Reference to AB-3 Developer Guide

@J0shuaLeong J0shuaLeong Nov 1, 2023

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 well done! Very organised and neat developer guide. The guide explains the application clearly with sufficient details and diagrams. I am sure more diagrams such as class diagrams would be added later on. Would also be good to add an embedded link to the java file for that specific component or command. For example, in the find command, can link the code file for quick access to the findcommand code.

Comment thread docs/DeveloperGuide.md
- Parser : Makes sense of user input
- Commands : List of various commands
- CommandResult : Execution of various commands

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a tip: Can try adding links to each component so the reader can quickly jump to that section instead of having to scroll through.

Comment thread docs/DeveloperGuide.md

The Parser class is designed to handle these considerations through a well-structured parsing process. Here's how it
works:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would be clearer to reader if you bold "Splitting User Input", "Command Recognition" etc.

Comment thread docs/DeveloperGuide.md
---

### Parser Component

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add a class diagram. Just a thought

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed here. Could use a diagram here

@James-Hong-Jey James-Hong-Jey 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 well done DG. Diagrams are well placed and detailed.

Comment thread docs/DeveloperGuide.md Outdated

The following sequence diagram shows how the login system class works when the program is launched.

<img src="UML Diagrams/StockerToLoginSystem.png" width="280">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very comprehensive and clear sequence diagram

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should the image be sized larger instead?

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 parser diagram look neat and correct

Comment thread docs/DeveloperGuide.md Outdated
## 5. Save Command

The save command was made as a means to backup user entered drug data into the hard drive of the computer to ensure
previously entered data is saved and accessable whenever the app is launched.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would a diagram demonstrating the Saving mechanism be helpful?

Comment thread docs/DeveloperGuide.md

The following sequence diagram shows how the Find Command function works.

<img src="UML Diagrams/FindCommandDiagram.png" width="350">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are the activation bars meant to terminate at that point? Or is the diagram cut off / incomplete?

Comment thread docs/DeveloperGuide.md
management system.

The following sequence diagram shows how the parser class works when the program is running.
<img src="UML Diagrams/ParserDiagram.png" width="500">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the 2nd Incorrect Command activation meant to be there? If so, is it intended to be in an Opt box?

Comment thread docs/DeveloperGuide.md

The following sequence diagram shows how the parser class works when the program is running.
<img src="UML Diagrams/ParserDiagram.png" width="500">

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 the creating of incorrect command is wrong. There should be a constructor and should return from the IncorrectCommand class instead of a self invocation.

Comment thread docs/DeveloperGuide.md
---

## 3. Delete Command

@J0shuaLeong J0shuaLeong Nov 1, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can consider adding sequence diagram for this command and the following commands as well except help and save. But i am guessing it is suppose to be done just haven't done it

Comment thread docs/DeveloperGuide.md
The architecture diagram given above explains the high level design of the application. The diagram depicts the key
component of the application that enables it to provide its functionalities.

Majority of the app's work is done by the following components

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 storage should be one of the components as well. Can add a sequence and class diagram for storage component.

Comment thread docs/DeveloperGuide.md Outdated

The following sequence diagram shows how the login system class works when the program is launched.

<img src="UML Diagrams/StockerToLoginSystem.png" width="280">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should the image be sized larger instead?

Comment thread docs/DeveloperGuide.md
The following sequence diagram shows how the Find Command function works.

<img src="UML Diagrams/ListCommandDiagram.png" width="350">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the ListCommand persist after displaying results?

Comment thread docs/DeveloperGuide.md Outdated

The following sequence diagram shows how the Command Result function works.

<img src="UML Diagrams/CommandResultDiagram.png" width="350">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do the command results and commands persist after calling?

Comment thread docs/DeveloperGuide.md
and criterion as input.

3. **Search Process:** The method processes the search by iterating through the list of `StockEntry` objects 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.

Simple search algorithm that minimises errors and allows for better scoping. Very apt and functional!

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Nov 13, 2023
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.