Skip to content
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-T15-1] CLIAlgo #33

Open
wants to merge 772 commits into
base: master
Choose a base branch
from

Conversation

ong-ck
Copy link

@ong-ck ong-ck commented Mar 2, 2023

CLIAlgo is a desktop application for managing your CS2040C notes. It is optimized to be used via a Command Line Interface (CLI). If you can type fast, you can access and sort your notes faster than ever before.

Copy link

@chao2048 chao2048 left a comment

Choose a reason for hiding this comment

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

The DG is pretty nice and clear overall!

### Initializing previous saved data feature
#### Current implementation

![](.\\sequence\\diagrams\\InitializationFileManager.png "FileManager Initialization Sequence Diagram")

Choose a reason for hiding this comment

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

Is the activation bar for the FileDecoder a bit too long?

### Export feature
#### Current implementation

![](.\\sequence\\diagrams\\Export.png "Export Sequence Diagram")

Choose a reason for hiding this comment

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

I think you can explain more about the part of Parser to FilterByTopicCommand to TopicManager, since they are shown on the diagram. Or perhaps it is not that important and can be ignored in the sequence diagram.

Here is a class diagram of the `TopoCommand` which facilitates the storage
function of the application.

![](.\\uml\\diagrams\\TopoCommandClass.png "TopoCommand Class Diagram")

Choose a reason for hiding this comment

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

The FileManager doesn't seem to be very relevant to the class diagram, and it makes the lines crossing each other. Perhaps it doesn't have to be in this diagram.

#### Current implementation

![](.\\sequence\\diagrams\\InitializationFileManager.png "FileManager Initialization Sequence Diagram")

Choose a reason for hiding this comment

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

I think you might need an arrow back from SingleFile back to FileManager after the readFile().

Copy link

@GraceZhuXY GraceZhuXY left a comment

Choose a reason for hiding this comment

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

The DG is detailed and easy to read! Great job 👍


### Ui
**API** : Ui.java
Here is a class diagram of the `Ui` component which is responsible for handling all interactin with the User.

Choose a reason for hiding this comment

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

There seems to be a small typo here ("interactin")


The following sequence diagram shows how the Parser work.

![](.\\sequence\\diagrams\\Parser.png "Parser Sequence Diagram")

Choose a reason for hiding this comment

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

The step-by-step description of the Parser is easy to understand! Perhaps the sequence diagram can also be simplified such that it is easier to comprehend together with the description (e.g. by removing return arrows, which is easy to infer)? This could also be applied to the sequence diagrams for filtering by topic operation.

> **Step 2**: The `parse()` method extracts out the command keyword provided by the user. It then calls the
> `prepareCommand()` method.

> **Step 3**: The `parepareCommand()` identifies the correct `Command` object to prepare based on the command keyword.

Choose a reason for hiding this comment

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

There seems to be a small typo here ("parepareCommand()")

@@ -2,37 +2,615 @@

## 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}

Choose a reason for hiding this comment

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

I think it would be great if there was a link to your UG/a brief overview of what this app is programmed to do! It would make it easier see all the following technical details in the bigger picture.

Here is a class diagram of the `FileManager` which facilitates the storage
function of the application.

![](.\\uml\\diagrams\\FileManagerClass.png "FileManager Class Diagram")

Choose a reason for hiding this comment

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

I think the diagram's focus would be clearer without the Scanner and FileWriter classes, as their functions can be described using labels.

@@ -2,37 +2,615 @@

## 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}

Copy link

Choose a reason for hiding this comment

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

I think it would be better to include a table of contents and links to the headers for easier navigation


The following sequence diagram shows how the Parser work.

![](.\\sequence\\diagrams\\Parser.png "Parser Sequence Diagram")
Copy link

Choose a reason for hiding this comment

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

image

Good job on the comprehensiveness of the diagram but the picture is unclear. maybe you can try dissecting this part into smaller sections


The following sequence diagram shows how previously saved files are loaded into `CLIAlgo`.

![](.\\sequence\\diagrams\\InitializationFileManager.png "FileManager Initialization Sequence Diagram")
Copy link

Choose a reason for hiding this comment

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

image

i think the activation bar inside the alt block should not be broken into two parts

Choose a reason for hiding this comment

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

May be try to modify the diagram to ensure the lines are not crossing each other

Choose a reason for hiding this comment

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

The activation bar of [isCorrupted] should not be disconnected

@@ -2,37 +2,614 @@

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

Choose a reason for hiding this comment

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

Should this section have links to your references?

Choose a reason for hiding this comment

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

Should the AddCommand and InvalidComment be shown at the end of the diagram when a red cross deletes the instance? This applied similarly to your other sequence diagrams.

Choose a reason for hiding this comment

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

Should the command class also have two blank sections for attribute and method like other classes?


1. Ensure that you have Java 11 or above installed.
1. Down the latest version of `Duke` from [here](http://link.to/duke).
## Features

Choose a reason for hiding this comment

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

Should the features section be placed in the user guide instead?

nicholas132000 and others added 24 commits April 3, 2023 11:22
…into branch-v2.0b-DG

# Conflicts:
#	docs/sequence-diagrams/diagrams/AddFeature.png
#	docs/sequence-diagrams/source-files/AddFeature.puml
Add design and implementation for remove and update add sequence diagram
heejet and others added 30 commits April 9, 2023 15:29
Final Refactor of UG, DG and PPP v2.1
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.

10 participants