[CS2113T-W11-1] SysLib CLI by yingx9 · Pull Request #6 · nus-cs2113-AY2324S1/tp

yingx9 · 2023-10-04T03:29:45Z

SysLib CLI is a program designed specially for system librarians to manage their work and
responsibilities. Using intuitive commands, view, add, delete, and find books from the library
inventory without any hassle.

…unit-tests Add JUnit tests for flashcards

Musfirahe0556596

Good! But here are some code quality violations to consider (I have also created Issues for each individual member)

Musfirahe0556596 · 2023-10-27T17:43:23Z

+
+public class Parser {
+
+    public List<Resource> resourceList = new ArrayList<>();


Plural form should be used on names representing a collection of objects.

Musfirahe0556596 · 2023-10-27T17:43:41Z

+        boolean matchFound = matcher.find();
+
+        if (matchFound) {
+            if (matcher.group(2).equalsIgnoreCase("b")) {


"b" is a magic literal - would be good if its placed as a named constant

Musfirahe0556596 · 2023-10-27T17:44:30Z

+            Matcher gMatcher = gPattern.matcher(matcher.group(5));
+            boolean gMatchFound = gMatcher.find();
+
+            String[] args = new String[6];
+
+            if (matchFound) {
+                args[0] = matcher.group(1).trim(); // id
+                args[1] = matcher.group(2).trim(); // title
+                args[2] = matcher.group(3).trim(); // author
+                args[3] = matcher.group(4).trim(); // tag
+                if (gMatchFound) {
+                    args[4] = gMatcher.group(1).trim(); // isbn
+                    args[5] = gMatcher.group(2).trim(); // genre
+                } else {
+                    args[4] = matcher.group(5).trim(); // isbn


Magic literals (e.g. group(<5>)) - would be good if the numbers are placed as a named constant.

Musfirahe0556596 · 2023-10-27T17:46:17Z

+            put("add", new AddCommand());
+        }
+    };
+    public void process(String response) {


Could rename the method process() better (e.g. process what?)

Musfirahe0556596 · 2023-10-27T17:46:41Z

+    }
+    public int parseInt(String value){
+        try {
+            int num = Integer.parseInt(value);


Could rename the variable num better to indicate what it is

Musfirahe0556596 · 2023-10-27T17:49:00Z

+        String[] value = parseArgument(statement);
+        validate(statement, value);
+
+        if (value[3]==null && value[2]==null && value[1]==null && value[0]==null) {


Magic literals e.g. value[<3>]- would be good if it is a named constant.

Musfirahe0556596 · 2023-10-27T17:49:07Z

+        ArrayList<Resource> matchedResources = new ArrayList<>();
+        for (Resource r: parser.resourceList){
+            Book b = (Book) r;
+            if (b.getTitle().equals(value[3]) || b.getISBN().equals(value[1]) || b.getAuthor().equals(value[2])){


Magic literals e.g. value[<3>]- would be good if it is a named constant.

Musfirahe0556596 · 2023-10-27T17:50:00Z

+    /*
+    validate will include str
+     */
+    public void validate(String statement, String[] value) throws IllegalArgumentException{


Could rename the method validate() better (e.g. validate what?)

Musfirahe0556596 · 2023-10-27T17:50:22Z

+    /*
+    validate will include str
+     */
+    public void validate(String statement, String[] value) throws IllegalArgumentException{


The argument String[] value should be in plural form.

Musfirahe0556596 · 2023-10-27T17:50:48Z

+    public void execute(String statement, Parser parser) throws SysLibException {
+        int id = parseInt(parseArgument(statement)[0]);
+        assert id > 0;
+        ArrayList<Resource> toRemove = new ArrayList<>();


Plural form should be used on names representing a collection of objects.

mcmc101001 · 2023-11-01T04:18:25Z

would it be better for the arrow in the condition where containsKey is false to show some kind of exception being invoked?

mcmc101001 · 2023-11-01T04:19:43Z

Would it be better to have some textual explaination of notEqualsIgnoreCase in the developer guide as it is quite ambiguous from the diagram what purpose it serves

mcmc101001 · 2023-11-01T04:23:55Z

Should the containsKey == false section be below the portion with the Command portion, because the Command portion only comes into play in the case where containsKey is true. Currently the containsKey == false portion is overlapping with the Command block

mcmc101001 · 2023-11-01T04:29:00Z

+
+`add` has seven options:
+- add /id [id] /t [title] /a [author] /tag [tag] /i [isbn]
+- add /id [id] /t [title] /a [author] /tag [tag] /i [isbn] _/g [genre] /s [status]_


What does the italics represent? Perhaps more explaination can be added to the DG to explain it. I am not sure if it is there to highlight the difference or to indicate that they are optional tags.

yicheng-toh

Good job on the DG.

yicheng-toh · 2023-11-02T09:31:54Z

One of the bar has a return arrow while another doesn't. Perhaps you could add the arrow in the second circle or you could add return type in the first circle.

yicheng-toh · 2023-11-02T09:34:29Z

Should the else box be extended all the way?

antrikshdhand

Overall a nice DG, however the verbose sequence diagrams could be improved.

antrikshdhand · 2023-11-02T09:40:49Z

+
+#### Sequence Diagram
+
+<img src="images/FindSequenceDiagram.png" />


This sequence diagram is quite verbose. All return values and method calls are written with sample arguments provided, which is not necessary for a high-level diagram.

antrikshdhand · 2023-11-02T09:42:32Z

+
+#### Sequence Diagram
+The following sequence diagram shows how the add function works:
+<img src="images/AddSequenceDiagram.png"/>


It's a good diagram in many ways, however the size could be much more compact without low-level details such as exactly what method is called or what text is passed into each argument.

antrikshdhand · 2023-11-02T09:43:16Z

+Step 1. The user inputs the command: `add /id 0005 /t Frankenstein /a Mary Shelley /i FKS0001 /tag B /g Gothic, Fiction`
+
+Step 2. The `UI` component forwards the input to `SYSLIB`, which in turn passes it to the `PARSER`.
+
+Step 3. The `PARSER` processes the command and determines that it contains a valid key (`add`). It then calls 
+`ADDCOMMAND#execute(statement: String, this: Parser)` with the input command.
+
+Step 4. The `ADDCOMMMAND` component receives the command and performs the following operations:
+- Calls `ADDCOMMAND#parseArgument(statement: String)` to extract values for ID, title, author, ISBN, tag, and genres.
+- Calls `ADDCOMMAND#validate(statement: String, values: String[])` to ensure the validity of the input command.
+
+Step 5. The `COMMAND` component processes the input command to ensure that it meets the required format and constraints.
+It prepares the argument values for further processing.
+
+Step 6. Since the `tag` argument in the input command indicates that it is a book, the `ADDCOMMAND` determines that the
+key is equal to `b` (ignoring case). It then creates a new `Book` object using the parsed values (title, ISBN, author, 
+genres, ID).
+
+Step 7. The newly created book is forwarded to the `PARSER` to be added to the `resourceList`.


A well-written, detailed explanation of the process. Great for a future developer!

skylee03

LGTM!

skylee03 · 2023-11-02T09:05:09Z

-## Design & implementation
+## Setting Up & Getting Started
+
+1. Fork the repo at https://github.com/AY2324S1-CS2113T-W11-1/tp.


Would it be better to use hyperlinks?

skylee03 · 2023-11-02T09:07:39Z

+- `UI`: User Interaction
+- `Parser`: Parsing User Response 
+- `Command`: Command Executor
+- `Data`: Holds the data of SysLib


Would it be better to indicate where other packages (e.g., storage, common, etc.) belong?

skylee03 · 2023-11-02T09:10:03Z

+- `find /id [ID]`
+- `find /t [TITLE]`
+- `find /a [AUTHOR]`
+- `find /i [ISBN]`


According to the textbook, we are suggested to use * instead of - for bullet-points, but it is optional to follow it.

skylee03 · 2023-11-02T09:11:11Z

+1. Parse the filters and their associated values.
+2. Filter the resources based on the given filters.
+3. Display the matching resources.


According to the textbook, we are suggested to use generic numbering for ordered lists, but it is optional to follow it.

skylee03 · 2023-11-02T09:14:41Z

+
+#### Example Usage Scenario
+
+Step 1. The user inputs the command: `add /id 0005 /t Frankenstein /a Mary Shelley /i FKS0001 /tag B /g Gothic, Fiction`


Why is "Step 1." in bold above but not here? Is it better to have a unified style?

skylee03 · 2023-11-02T09:17:34Z

This picture is too wide! Try to avoid repeating the parameters that are too long.

One activation bar corresponds to one method call. Therefore :Command should have 2 activation bars.

AddCommand is a subclass of Command, and the :AddCommand instance is just calling the methods of it self. Would it be more accurate to regard parseArgument and validateStatement as its members?

Would it be more accurate to explicitly mark :AddCommand as destroyed?

skylee03 · 2023-11-02T09:29:53Z

One activation bar corresponds to one method call. Therefore :Command should have 2 activation bars.

Would it be more accurate to use -> instead of --> for the arrow of warning?

FindCommand is a subclass of Command, and the :FindCommand instance is just calling the methods of it self. Would it be more accurate to regard parseArgument and validateStatement as its members?

skylee03 · 2023-11-02T09:33:18Z

One activation bar corresponds to one method call. Therefore :Command should have 2 activation bars.

Would it be more accurate to complete the return arrows?

skylee03 · 2023-11-02T09:37:41Z

The table is not displayed properly. Please try to add an empty line between the table and the # About us.

The avatars are too big! Maybe making the picture smaller will look better!

skylee03 · 2023-11-02T09:40:00Z

+ ____            _     _ _        ____ _     ___ 
+/ ___| _   _ ___| |   (_) |__    / ___| |   |_ _|
+\___ \| | | / __| |   | | '_ \  | |   | |    | | 
+ ___) | |_| \__ \ |___| | |_) | | |___| |___ | | 
+|____/ \__, |___/_____|_|_.__/   \____|_____|___|
+       |___/                                     


Spectacular ASCII art!

KenCheung18 · 2023-11-02T09:10:36Z

+
+SysLib currently consists of four main components:
+
+- `UI`: User Interaction


UI should be User Interface?

KenCheung18 · 2023-11-02T09:19:01Z

+### UI Component
+
+### Parser Component
+
+### Command Component
+
+
+### Data Component


Maybe can add some content after each title

Maybe can add some content after each title

I think they just have not started to write them… 😂

KenCheung18 · 2023-11-02T09:36:58Z

+
+#### Sequence Diagram
+
+<img src="images/FindSequenceDiagram.png" />


Should add a conditional block for returning legalArgumentException? Since it's try catch block so not necessary to throw exception

KenCheung18 · 2023-11-02T09:47:12Z

+
+#### Sequence Diagram
+
+<img src="images/FindSequenceDiagram.png" />


if you are calling a function resourceList should be like resourceList() in the sequence diagram

KenCheung18 · 2023-11-02T09:52:25Z

+Commands available:
+add: adds a new resource to the library inventory.(e.g. add /id ID /t TITLE /a AUTHOR /tag TAG /i ISBN [/g GENRE])
+delete: deletes the resource with the specified ID from the library inventory. (e.g. delete /id 123456789)
+list: list all resources OR filter by certain tags or genre.(e.g. list /tag B /g Fiction
+find: find a resource by title, author, ISBN or given id. (e.g. find /i 9780763630188)
+edit: Edit a listing by entering its isbn to update its details. (e.g. edit /i 123 /t NEW_TITLE /a NEW_AUTHOR)
+exit: displays a farewell message and exits the program (e.g. exit)
+For more information, please refer to our user guide at:https://ay2324s1-cs2113t-w11-1.github.io/tp/UserGuide.html
+____________________________________________________________
+```


A bit hard to read

junhyeong0411 · 2023-11-03T03:15:25Z

When you use exception on sequence diagram, I think using Alt block will be better.
Then you can also show that the activation bar brokes because of IllegalArgumentException.

junhyeong0411 · 2023-11-03T03:20:20Z

In these parts, I think it is better to use alt blocks to show if-else statement.
Those bars are self-invocation bars, but actually your code doesn't self-invoke in that part.

junhyeong0411 · 2023-11-03T03:27:26Z

When writing components name, I think matching the Uppercase and Lowercase with the actual code is better.
(For example, Ui, Parser, ...)

junhyeong0411 · 2023-11-03T03:29:21Z

In this part, I think user can be more detailed.

TongZhengHong · 2023-11-02T09:29:05Z

+- `Command`: Command Executor
+- `Data`: Holds the data of SysLib
+
+


Consider adding class diagrams to show how the different classes are related to each other.

TongZhengHong · 2023-11-02T09:33:08Z

+#### Sequence Diagram
+The following sequence diagram shows how the add function works:
+<img src="images/AddSequenceDiagram.png"/>
+


This diagram contains a repeated input string which is very long. Can consider mentioning the input outside of the diagram to make the diagram more compact and readable

TongZhengHong · 2023-11-02T09:38:31Z

+<img src="images/FindSequenceDiagram.png" />
+
+### Examples for Testing
+


Good use of examples!

TongZhengHong · 2023-11-02T09:40:21Z

+#### Implementation
+
+It implements the following operations:
+


Is ADDCOMMAND needed for each operation? Might want to explicitly mention that each operation belongs to ADDCOMMAND once at the start instead

TongZhengHong · 2023-11-02T09:42:50Z

+
+`add` has seven options:
+- add /id [id] /t [title] /a [author] /tag [tag] /i [isbn]
+- add /id [id] /t [title] /a [author] /tag [tag] /i [isbn] _/g [genre] /s [status]_


Can consider putting these commands in a code block for readability.

…aster Update

Update Developer Guide

Created a Message.java class under UI to hold messages that are meant to be output to the user. Made minor update to DG.

Check Code Quality and Coding Standards

Update help message for edit and list

Remove unnecessary new lines and unused variables to improve neatness and quality

fix bug for test

fix missed bugs

Update DG

# Conflicts: # docs/DeveloperGuide.md

…oanneAng-PPP # Conflicts: # docs/DeveloperGuide.md

Update Project Portfolio Page

# Conflicts: # src/test/java/seedu/commands/AddCommandTest.java

Update DG usecase and UG

Add More JUnit Tests

…adMe

Fix ReadMe

Update Project Portfolio Page

Add Syslib version printout

Update UG

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Oct 13, 2023

Merge pull request nus-cs2113-AY2324S1#6 from wendelinwemhoener/add-j…

79d78c7

…unit-tests Add JUnit tests for flashcards

Musfirahe0556596 reviewed Oct 27, 2023

View reviewed changes

mcmc101001 reviewed Nov 1, 2023

View reviewed changes

yicheng-toh reviewed Nov 2, 2023

View reviewed changes

antrikshdhand reviewed Nov 2, 2023

View reviewed changes

skylee03 reviewed Nov 2, 2023

View reviewed changes

KenCheung18 reviewed Nov 2, 2023

View reviewed changes

TongZhengHong reviewed Nov 3, 2023

View reviewed changes

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Nov 6, 2023

Merge pull request nus-cs2113-AY2324S1#6 from AY2324S1-CS2113-F11-3/m…

036d5b4

…aster Update

yingx9 and others added 17 commits November 11, 2023 13:31

Merge pull request #174 from JoanneJo/JoanneAng-DeveloperGuide

6854f6d

Update Developer Guide

Check Code Quality and Coding Standards

3312f69

Created a Message.java class under UI to hold messages that are meant to be output to the user. Made minor update to DG.

Update EXPECTED.TXT

53e1fe9

Merge pull request #175 from JoanneJo/JoanneAng-CodeQualityStandards

0bbbeee

Check Code Quality and Coding Standards

Add Messages class specific to EditCommand and ListCommand

78762a9

Update help message for edit and list

Update EditCommand for minor change

dc4513d

Fix checkstyle error

9b6f673

Improve coding standard and quality

e926aa0

Remove unnecessary new lines and unused variables to improve neatness and quality

Update User Guide

f3fddd7

Fix user guide images

35e3122

Update DG

95d285d

Fix wrong argument message error for CD

77b5dac

Fix bugs in code and UG

a3be979

Update PPP and code quality

c34bd03

Improve code quality for Find Command

9b7653c

Remove unused code

13206cc

Fix checkstyle error

d2f49f3

DavinciDelta and others added 30 commits November 14, 2023 10:35

Update EXPECTED.TXT

447388e

fix bug for test

Update EXPECTED.TXT

8a682db

fix missed bugs

Merge pull request #217 from DavinciDelta/master

2ce2014

Update DG

Merge remote-tracking branch 'upstream/master' into JoanneAng-PPP

21003a1

# Conflicts: # docs/DeveloperGuide.md

Update DG usecase and UG

d2c5ffa

Merge branch 'JoanneAng-PPP' of https://github.com/JoanneJo/tp into J…

d0c820d

…oanneAng-PPP # Conflicts: # docs/DeveloperGuide.md

Merge pull request #215 from JoanneJo/JoanneAng-PPP

a8b589e

Update Project Portfolio Page

Merge branch 'master' into Update-DG

1bec5c7

Merge branch 'JoanneAng-PPP' into JoanneAng-AddCommandJUnit

2e1ae79

# Conflicts: # src/test/java/seedu/commands/AddCommandTest.java

Merge pull request #218 from 000verflow/Update-DG

e1d3486

Update DG usecase and UG

Merge pull request #216 from JoanneJo/JoanneAng-AddCommandJUnit

274a634

Add More JUnit Tests

Fix logo link

0d39e40

Merge pull request #219 from yingx9/YingXia-FixLogoReadMe

de188eb

Fix logo formatting

3fcb1d6

Merge remote-tracking branch 'upstream/master' into YingXia-FixLogoRe…

3e4b854

…adMe

Fix images and formatting error

4457e1b

Fix formatting error

7cfd867

Update DG and UG formatting

026b4b9

Update Project Portfolio Page

09d40b6

Prints Syslib version

f77ddfb

Merge pull request #220 from yingx9/YingXia-FixReadme

d09005f

Fix ReadMe

Merge pull request #221 from JoanneJo/JoanneAng-PPP

b9677d3

Update Project Portfolio Page

Update test

c886ae1

Fix testp1

657f857

Fix testp2

0f84321

fix test

d857fbe

Merge pull request #222 from bnjm2000/BenjaminNg-addVersion

3d0f6b3

Add Syslib version printout

Merge remote-tracking branch 'upstream/master' into JoanneAng-UserGuide

e2992e2

Update UG

4169708

Merge pull request #223 from JoanneJo/JoanneAng-UserGuide

697c7ee

Update UG


		public class Parser {

		public List<Resource> resourceList = new ArrayList<>();


		#### Sequence Diagram

		<img src="images/FindSequenceDiagram.png" />


		#### Example Usage Scenario

		Step 1. The user inputs the command: `add /id 0005 /t Frankenstein /a Mary Shelley /i FKS0001 /tag B /g Gothic, Fiction`


		SysLib currently consists of four main components:

		- `UI`: User Interaction

		- `Command`: Command Executor
		- `Data`: Holds the data of SysLib

		<img src="images/FindSequenceDiagram.png" />

		### Examples for Testing

Uh oh!

Conversation

yingx9 commented Oct 4, 2023

Uh oh!

Musfirahe0556596 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yicheng-toh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antrikshdhand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skylee03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Musfirahe0556596 left a comment •

edited

Loading