Skip to content

[CS2113-T16-1] Libmgr#38

Open
exetr wants to merge 515 commits into
nus-cs2113-AY2122S1:masterfrom
AY2122S1-CS2113-T16-1:master
Open

[CS2113-T16-1] Libmgr#38
exetr wants to merge 515 commits into
nus-cs2113-AY2122S1:masterfrom
AY2122S1-CS2113-T16-1:master

Conversation

@exetr

@exetr exetr commented Sep 30, 2021

Copy link
Copy Markdown

Libmgr helps librarians to streamline the process of managing the book catalogues within their library.

Comment thread docs/DeveloperGuide.md Outdated
## Design & implementation

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
This section provides an overview of the design architecture and implementation of Libmgr. Each sub-section provides a detailed explanation of the design of each 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.

Missing introduction and explanation of program?

Comment thread docs/DeveloperGuide.md
### Entrypoint of Libmgr

![InitializationMainFunction](img/InitializationMainFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't print(LOGO) and print(WELCOME) create another activation bar of the class?
image

Comment thread docs/DeveloperGuide.md Outdated
The commands component consists of a 'commands' package which holds a main Parser class to execute all the commands, as well as
individual class files, each corresponding to a specific command, that inherit from an abstract command class.

![ParserAndCommandClassDiagram](img/ParserAndCommandClassDiagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Diagram is too small, unable to read the words in it

Comment thread docs/DeveloperGuide.md

The data component consists of a `data` package which holds classes that aim to allow the categorisation of items into different types.

![ItemsClassDiagram](img/ItemsClassDiagram.png)

@yanjia1777 yanjia1777 Oct 27, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Labels are vague, don't know what they are trying to represent
image

Comment thread docs/DeveloperGuide.md Outdated

### Entrypoint of Libmgr

![InitializationMainFunction](img/InitializationMainFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adding a User bar on the left makes it easier for the reader to know where the program flow begins.

image

Comment thread docs/DeveloperGuide.md Outdated
The commands component consists of a 'commands' package which holds a main Parser class to execute all the commands, as well as
individual class files, each corresponding to a specific command, that inherit from an abstract command class.

![ParserAndCommandClassDiagram](img/ParserAndCommandClassDiagram2.png)

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 class diagram seems to have too much information and as a result the text size is very small, it looks blurry. Maybe try dividing the diagram into a few sub-sections and elaborating them.

image

@williamlamjy williamlamjy Oct 28, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In addition to this, it may not be necessary to list out every single parameter and method. U could omit out certain methods that might be redundant.

Comment thread docs/DeveloperGuide.md Outdated

### Data Package

The data component consists of a `data` package which holds classes that aim to allow the categorisation of items into different types.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adding more explanation about the technical workings of the module would help and giving a brief description of how classes interact.

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +13 to +15
### Entrypoint of Libmgr

![InitializationMainFunction](img/InitializationMainFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No description provided for Entrypoint of the program

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +17 to +19
### Searching feature of Libmgr

![SearchFunction](img/SearchFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No description provided for this section

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +6 to +9

## Design & implementation

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
This section provides an overview of the design architecture and implementation of Libmgr. Each sub-section provides a detailed explanation of the design of each 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.

Missing table of contents

@@ -0,0 +1,151 @@
@startuml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this seq diagram is way too big and with too many sections. The font is too small and most of the details is really hard to see. Maybe u could simplify the diagram into many different diagrams. The first diagram could have the basic entry to the if-else statements and subsequently u can separate the 4 cases into 4 separate diagrams.

Comment thread docs/DeveloperGuide.md Outdated
The commands component consists of a 'commands' package which holds a main Parser class to execute all the commands, as well as
individual class files, each corresponding to a specific command, that inherit from an abstract command class.

![ParserAndCommandClassDiagram](img/ParserAndCommandClassDiagram2.png)

@williamlamjy williamlamjy Oct 28, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In addition to this, it may not be necessary to list out every single parameter and method. U could omit out certain methods that might be redundant.

Comment thread docs/DeveloperGuide.md Outdated
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
This section provides an overview of the design architecture and implementation of Libmgr. Each sub-section provides a detailed explanation of the design of each component

### Architecture 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.

Would be good to include an architecture diagram of the overall program. :)

Comment thread docs/DeveloperGuide.md Outdated

### Searching feature of Libmgr

![SearchFunction](img/SearchFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screenshot 2021-10-28 at 2 20 01 PM
It seems like this is a bit too deeply nested. May be you can consider using guard clauses to make the main path clearer.

Comment thread docs/DeveloperGuide.md Outdated
### Commands Package

The commands component consists of a 'commands' package which holds a main Parser class to execute all the commands, as well as
individual class files, each corresponding to a specific command, that inherit from an abstract command class.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screenshot 2021-10-28 at 2 39 32 PM
Can consider using code format of some words like Parser.

Comment thread docs/DeveloperGuide.md Outdated

#### Edit Command

The Edit Command class handles the functionality to change a specific detail of an item in the catalogue.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screenshot 2021-10-28 at 2 41 27 PM
There could be a little bit more explanation about the logic here based on the detailed sequence diagram below. One point that I can think of would be different instances eg magazine, audio that the command can edit,

@weidak weidak 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 good effort on the developer guide! 👍 Keep up the good work :D

Comment thread docs/DeveloperGuide.md Outdated
## Design & implementation

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
This section provides an overview of the design architecture and implementation of Libmgr. Each sub-section provides a detailed explanation of the design of each 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.

There's a missing architecture diagram after this statement, and more information can be provided on the overall design of the application.

Comment thread docs/DeveloperGuide.md Outdated

### Entrypoint of Libmgr

![InitializationMainFunction](img/InitializationMainFunctionSequence.png)

@weidak weidak Oct 29, 2021

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 method calls are missing for the methods outside of the 'loop' block.

I think the deletion cross is on top of the activation bar for Command and maybe it can be moved down slightly :D
image

There should be a self-invocation method (inner activation bar) here as well.
image

Comment thread docs/DeveloperGuide.md Outdated

### Searching feature of Libmgr

![SearchFunction](img/SearchFunctionSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

I believe this was to ensure clarity of the search functionality of the implementation. However, would it be better if you were to extract these out and put them as reference frames? I believe they are very similar in terms of each if-else loop so 1 arbitrary reference frame can bring the idea across clearly without a huge UML Diagram 👍

Comment thread docs/DeveloperGuide.md Outdated
The commands component consists of a 'commands' package which holds a main Parser class to execute all the commands, as well as
individual class files, each corresponding to a specific command, that inherit from an abstract command class.

![ParserAndCommandClassDiagram](img/ParserAndCommandClassDiagram2.png)

@weidak weidak Oct 29, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

As mentioned previously by another reviewer, I think maybe omitting the methods and attributes would be better for clarity and understanding of the picture because right now I think its overwhelming to the users to read.

Comment thread docs/DeveloperGuide.md Outdated

The Edit Command class handles the functionality to change a specific detail of an item in the catalogue.

![EditCommandSequence](img/EditCommandSequence.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

image

Similarly to my comment above, I believe you can put these alt paths into smaller arbitrary reference frames to make it clearer to the readers and this makes the diagram less overwhelming in general.

exetr and others added 25 commits October 31, 2021 20:40
Fix broken table of content links
Updated the search feature to intake multiple parameter
Add error handling in extractArgs for malformed inputs
# Conflicts:
#	src/main/java/seedu/duke/commands/Parser.java
silinche and others added 30 commits November 8, 2021 21:18
Add authorship and fix some typos
Created PPP for Silin, Updated sequence diagram for search function
Fix documentation typos
Update PPP and minor change
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