[CS2113-W14-4] Pharmacy Inventory & Logistics Ledger (PILL) #28

yijiano · 2024-10-01T04:36:56Z

Pharmacy Inventory & Logistics Ledger (PILL) is a Command Line Interface (CLI) tool designed to assist in
managing and tracking medicinal inventory.

thomasjlalba

Do remember to ensure your code is more readable for others and to simplify expressions. Keep up the good work

thomasjlalba · 2024-10-25T08:56:44Z

src/main/java/seedu/pill/command/HelpCommand.java

+        logger.info("Showing specific help for command: " + command);
+
+        switch (command.toLowerCase()) {
+        case "help":


Maybe you can refactor the magic string?

thomasjlalba · 2024-10-25T09:00:21Z

src/main/java/seedu/pill/util/StringMatcher.java

+                } else if (j == 0) {
+                    dp[i][j] = i;
+                } else {
+                    dp[i][j] = min(dp[i - 1][j - 1]


Maybe you can look to refactor this line to make it more readable?

thomasjlalba · 2024-10-25T09:01:58Z

src/main/java/seedu/pill/util/StringMatcher.java

+        String closestMatch = null;
+        int minDistance = Integer.MAX_VALUE;
+
+        for (String valid : validStrings) {


valid might be a bit ambiguous, maybe you can use validString instead?

thomasjlalba · 2024-10-25T09:08:08Z

src/main/java/seedu/pill/util/Storage.java

+    public Item loadLine(String line) throws PillException {
+        Item item;
+        String[] data = line.split(SEPARATOR);
+        if (data.length == 3) {


Maybe you can refactor the magic literal to make it more readable

thomasjlalba · 2024-10-25T09:09:34Z

src/main/java/seedu/pill/util/Storage.java

+        String[] data = line.split(SEPARATOR);
+        if (data.length == 3) {
+            try {
+                item = new Item(data[0], Integer.parseInt(data[1]), LocalDate.parse(data[2]));


Maybe you could refactor data[0] and so on such that it is more understandable what each variable means before the if statement

thomasjlalba · 2024-10-25T09:27:52Z

src/main/java/seedu/pill/util/ItemMap.java

+            List<Item> filteredItems = items.values().stream()
+                    .flatMap(TreeSet::stream)
+                    .filter(item -> item.getQuantity() <= threshold)
+                    .toList();


Maybe you can look into refactoring this to make it more readable and less complicated?

thomasjlalba · 2024-10-25T09:28:55Z

src/main/java/seedu/pill/util/Parser.java

+            String arguments = String.join(" ", Arrays.copyOfRange(splitInput, 1, splitInput.length));
+
+            switch (commandString) {
+            case "exit":


Maybe you can look into refactoring these magic strings?

thomasjlalba · 2024-10-25T09:33:23Z

src/main/java/seedu/pill/command/DeleteItemCommand.java

+
+    @Override
+    public void execute(ItemMap itemMap, Storage storage) throws PillException {
+        // Looks stupid, but this way I don't handle Optionals in this class


Maybe you can rephrase this to write comments targeting other programmers reading the code

thomasjlalba · 2024-10-25T09:35:10Z

src/main/java/seedu/pill/command/HelpCommand.java

+            } else {
+                suggestSimilarCommand(command);
+            }


Maybe this can be changed to a guard clause instead to make the happy path more prominent

thomasjlalba · 2024-10-25T09:36:13Z

src/main/java/seedu/pill/command/HelpCommand.java

+    private final String commandName;
+    private final boolean verbose;
+
+    public HelpCommand(String commandInput, boolean verbose) {


Maybe you can change the boolean variable name to make it more obvious it is a boolean

Bev-low

Overall looks neat

Bev-low · 2024-10-30T02:36:52Z

docs/DeveloperGuide.md

+
+<!-- @@author yakultbottle -->
+
+### Storage


I think these classes can have UML diagrams

Bev-low · 2024-10-30T02:38:29Z

docs/DeveloperGuide.md

+Example:
+
+```
+AddItemCommand command = new AddItemCommand(itemName, quantity, expiryDate);


I think this can be put in the implementation section, following the example they give us.

Bev-low · 2024-10-30T02:39:35Z

docs/DeveloperGuide.md


+**API**: PillLogger.java
+
+<img src = "diagrams/PillLogger.png" alt="Component Diagram for PillLogger"/>


This diagram looks like it doesnt follow the standard

yeah, i think missing some tag and some place should use dotted lines

Bev-low · 2024-10-30T02:40:41Z

docs/DeveloperGuide.md

+The ItemMap class contains a key-value pair, implemented as a Map, from the
+item name(String) to the item(TreeSet\<Item>)
+
+![](diagrams/ItemMap-ClassDiagram.png)


I dont think this follows the standard also, I am not sure what the dotted box "item" mean on comparable

should do comparable:item

YukeeHong · 2024-10-30T02:52:24Z

docs/DeveloperGuide.md

+
+The Item class has three private variables, a name, a quantity, and an
+expiry date. An Item may or may not have an expiry date, so we store it
+as an Optional, which handles empty values for us without using null. 


could briefly explain why Optional was chosen over null

YukeeHong · 2024-10-30T02:55:30Z

docs/DeveloperGuide.md


+**API**: PillLogger.java
+
+<img src = "diagrams/PillLogger.png" alt="Component Diagram for PillLogger"/>


yeah, i think missing some tag and some place should use dotted lines

YukeeHong · 2024-10-30T02:56:33Z

docs/DeveloperGuide.md

+* Technical Requirements: Any mainstream OS, i.e. Windows, macOS or Linux, with Java 11 installed. Instructions for downloading Java 11 can be found [here](https://www.oracle.com/sg/java/technologies/javase/jdk11-archive-downloads.html).
+* Project Scope Constraints: The application should only be used for tracking. It is not meant to be involved in any form of monetary transaction.
+* Project Scope Constraints: Data storage is only to be performed locally.
+* Quality Requirements: The application should be able to be used effectively by a novice with little experience with CLIs.


can be more detailed and concrete

YukeeHong · 2024-10-30T03:01:05Z

docs/DeveloperGuide.md

+
+#### AddItemCommand
+
+<img src = "diagrams/AddItemCommand-SequenceDiagram.png"/>


some part like print and log can be simplified

DanLinhHuynh-Niwashi · 2024-10-30T03:41:14Z

docs/DeveloperGuide.md

+
+#### AddItemCommand
+
+<img src = "diagrams/AddItemCommand-SequenceDiagram.png"/>


Would it be better to add some activation bars to show if the objects are working?

DanLinhHuynh-Niwashi · 2024-10-30T03:47:12Z

docs/DeveloperGuide.md

+
+The high-level overview of the project structure is as follows:
+
+<img src = "diagrams/High-Level-Overview.png" alt="High Level Overview of PILL"/>


You may try to consider the direction of arrows. Since it's is MVC model, the arrow from View to Controller might be a two way arrow (e.g. View send request to Controller and Controller response with data?), the same for Controller to Model components

DanLinhHuynh-Niwashi · 2024-10-30T03:51:25Z

docs/DeveloperGuide.md


 ## Design & implementation

-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+The project is designed using the Model-View-Controller (MVC) architecture, with the following components:


Since Model - Controller - View is at a higher level comparing to other classes (e.g. Model contains Item and ItemClass) and they are not really a class as well, you may try to create a hierarchy, just to distinguish between the architecture and the actual implementation. For example:
3. Controller:
3.1. Item
3.2. ItemMap

DanLinhHuynh-Niwashi · 2024-10-30T03:55:35Z

docs/DeveloperGuide.md

+
+The UI consists of the following components:
+
+1. **Parser**: The Parser class is responsible for interpreting user input and returning the corresponding command.


As I think UI is the 'View' part in your architecture, should it be better to make this parser a part of the Controller instead of View? As the Controller in MVC is responsible for handling user input, processing it, and coordinating between the Model and the View, I think the parser is nearer to a Controller component than a View component

DanLinhHuynh-Niwashi · 2024-10-30T03:57:37Z

docs/DeveloperGuide.md

+
+<!-- @@author cxc0418 -->
+
+### UI and I/O


Will it be good to put these implementation into a hierarchy corresponding to your architecture? To make a clearer view of how each component related to the high-level architecture!

Zackermax · 2024-10-30T05:15:33Z

docs/diagrams/High-Level-Overview.puml

The diagram should not have class symbols (green C) and the arrows should be orthogonal.

Zackermax · 2024-10-30T05:16:30Z

docs/diagrams/AddItemCommand-SequenceDiagram.puml

The diagram is missing activation boxes

Zackermax · 2024-10-30T05:19:36Z

docs/diagrams/PillLogger.puml

The arrows should be orthogonal.

Zackermax · 2024-10-30T05:21:27Z

docs/diagrams/AddItemCommand-SequenceDiagram.puml

The formatting for the "else" condition in the second section is not formatted properly.

LTK-1606

Looks neat, but notations used may not follow requirements, some broken links and maybe more diagrams would be better

LTK-1606 · 2024-10-30T05:20:47Z

docs/DeveloperGuide.md

+expiry date. An Item may or may not have an expiry date, so we store it
+as an Optional, which handles empty values for us without using null. 
+
+![](diagrams/Item-ClassDiagram.png)


Are your notations used correctly? Should it be "function(): 'return type'"?

LTK-1606 · 2024-10-30T05:24:10Z

docs/DeveloperGuide.md

+
+#### AddItemCommand
+
+<img src = "diagrams/AddItemCommand-SequenceDiagram.png"/>


Are these notations correct? Should it include activation bars?

LTK-1606 · 2024-10-30T05:28:15Z

docs/DeveloperGuide.md

+
+<!-- @@author cxc0418 -->
+
+### UI and I/O


Should there be more visuals and diagrams to make this clearer?

LTK-1606 · 2024-10-30T05:32:22Z

docs/DeveloperGuide.md

+        - [DateTime](#datetime)
+        - [Exceptions and Logging](#exceptions-and-logging)


Are these links supposed to be here? Should they be broken?

Implement Use command with priority removal

Update on the cost & price etc

Notes: - Added in-line comments for clarity.

Notes: - Added TimestampIO.java

Notes: - Added new test cases for coverage in HelpCommandTest.java

Notes: - Added TimestampIOTest.java

Timestamps, JUnit Tests, Comments // Implemented

PPP

Fix PPP formatting

Update high level architecture diagram

Fix DG and UG

UG and DG small fixes

minor change to PPP

Update PPP

Update PPP formatting

Add acknowledgement for XChart

thomasjlalba reviewed Oct 25, 2024

View reviewed changes

Bev-low reviewed Oct 30, 2024

View reviewed changes

YukeeHong reviewed Oct 30, 2024

View reviewed changes

DanLinhHuynh-Niwashi reviewed Oct 30, 2024

View reviewed changes

Zackermax reviewed Oct 30, 2024

View reviewed changes

LTK-1606 reviewed Oct 30, 2024

View reviewed changes

cxc0418 and others added 24 commits November 1, 2024 16:33

Merge branch 'master' of https://github.com/cxc0418/tp

ec508d4

Add execute method for UseItemCommand

cb91ae9

Update on UG

de27a32

Update on UG

94ea60c

Update on UG

2529dd4

Update execute method to follow convention in other ItemCommands

275f0f0

Add test coverage for UseItemCommand

b8b412d

Correct checkstyle errors

e2d1769

Merge pull request #135 from yakultbottle/priorityRemoval

6c52c01

Implement Use command with priority removal

Merge branch 'master' into master

296c33d

Resolve conflict

728ee91

Resolve conflict

2f6c4af

resolve conflict

0fa2805

Merge pull request #134 from cxc0418/master

915755c

Update on the cost & price etc

Comments // Updated

1e6ef05

Notes: - Added in-line comments for clarity.

TimestampIO // Implemented

01ba8f6

Notes: - Added TimestampIO.java

JUnit Tests // Updated

ca2de19

Notes: - Added new test cases for coverage in HelpCommandTest.java

TimestampIOTest // Implemented

44fbeab

Notes: - Added TimestampIOTest.java

Indentation // Updated

f1d0d4e

Variable Declaration // Fixed

7df5087

TimestampIO & TimestampIOTest // Updated

2e9d923

no message

86755b8

ASDFPLSPLSPLSPLSPLS

38663b3

Merge pull request #136 from philip1304/master

ad491f7

Timestamps, JUnit Tests, Comments // Implemented

philip1304 and others added 30 commits November 12, 2024 06:27

Merge branch 'AY2425S1-CS2113-W14-4:master' into master

e1b890c

Fix formatting

63d2b0c

Fix spacing

47ef899

Merge branch 'master' into bugfix/ppp-formatting

79aaa6e

Update spacing

e7ad875

Merge pull request #252 from philip1304/master

37428d2

PPP

Merge pull request #253 from cnivedit/bugfix/ppp-formatting

382d3b5

Fix PPP formatting

Merge pull request #249 from cnivedit/devguide/overview

1da3636

Update high level architecture diagram

Fix DG

1cffcbf

Fix UG, DG diagram

bbdb21e

Fix format

53ef016

Merge pull request #255 from yijiano/fix/DG-UG

c6bc3b9

Fix DG and UG

Fix UG formatting

b021016

Merge pull request #256 from yakultbottle/master

987baf9

UG and DG small fixes

8529d8c

Merge pull request #257 from yakultbottle/docs-triage

26e3adb

UG and DG small fixes

minor change to PPP

189b2f9

Merge pull request #258 from yakultbottle/final-PPP

befa9e5

minor change to PPP

Update PPP

646d95f

Merge branch 'master' into ppp/cnivedit-2

2a87165

Merge pull request #259 from cnivedit/ppp/cnivedit-2

d9a6765

Update PPP

Add acknowledgement for XChart

2fcd503

Add page break and image widths

edcdd92

Merge branch 'master' into ppp/cnivedit-3

f4c4083

Merge pull request #261 from cnivedit/ppp/cnivedit-3

eabb078

Update PPP formatting

Hopefully fix visualize

6535a0a

Merge branch 'master' into ack

24c0739

common checkstyle L

d282bb9

Merge pull request #260 from yakultbottle/ack

333ca47

Add acknowledgement for XChart

Update AboutUs.md

3bf9c1a


		API: PillLogger.java

		<img src = "diagrams/PillLogger.png" alt="Component Diagram for PillLogger"/>


		#### AddItemCommand

		<img src = "diagrams/AddItemCommand-SequenceDiagram.png"/>


		The high-level overview of the project structure is as follows:

		<img src = "diagrams/High-Level-Overview.png" alt="High Level Overview of PILL"/>


		The UI consists of the following components:

		1. Parser: The Parser class is responsible for interpreting user input and returning the corresponding command.

		- [DateTime](#datetime)
		- [Exceptions and Logging](#exceptions-and-logging)

[CS2113-W14-4] Pharmacy Inventory & Logistics Ledger (PILL) #28

Are you sure you want to change the base?

[CS2113-W14-4] Pharmacy Inventory & Logistics Ledger (PILL) #28

Conversation

yijiano commented Oct 1, 2024

thomasjlalba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bev-low left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zackermax Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

LTK-1606 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zackermax Oct 30, 2024 •

edited

Loading