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-F13-3] NUSplanner #52

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

Conversation

kyrixn
Copy link

@kyrixn kyrixn commented Mar 3, 2023

NUSplanner is a desktop app that allows for an easy and straightforward way for NUS students to manage their schedule ranging from person, school or external related activities. This application makes use of a desktop Command Line Interface (CLI), enabling a quick and sleek method of getting your schedule in check.

kyrixn pushed a commit to kyrixn/tp that referenced this pull request Mar 16, 2023

#### How the feature is implemented:

![Storage Class Diagram](UML/Images/StorageSequenceDiagram.png)

Choose a reason for hiding this comment

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

The diagram is good but it looks quite hectic. Consider breaking it up using reference frames with proper annotation for easier comprehension


The class diagram below illustrates the structure of the storage package

![Storage Class Diagram](UML/Images/StorageClass.png)

Choose a reason for hiding this comment

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

Simple and easy to read !


The class diagram below illustrates the structure of the EventList component.

<img src="UML\Images\EventListUML.png" />

Choose a reason for hiding this comment

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

Perhaps you should consider noting down the methods within the UI and Parser classes


And below is a sequential diagram showing a event being added, revised, checked for information and finally deleted.

<img src="UML\Images\EventListSD.png" style="zoom:80%;" />

Choose a reason for hiding this comment

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

Good job nice and easy to read !

Copy link

@Masahiro21 Masahiro21 left a comment

Choose a reason for hiding this comment

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

Overall the diagrams are very detailed and the explanations were easy to understand.

<img src="UML\Images\EventListUML.png" />

And below is a sequential diagram showing a event being added, revised, checked for information and finally deleted.

Choose a reason for hiding this comment

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

image
Not a bug but the arrow is out of place compared to the rest of diagram, to improve on the quality of the diagram you can fix it.

![Storage Class Diagram](UML/Images/StorageClass.png)

#### How the feature is implemented:

Choose a reason for hiding this comment

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

image
<> if file does not exist should be encased in the alt box since it only runs under the condition that the file does not exist

![Storage Class Diagram](UML/Images/StorageClass.png)

#### How the feature is implemented:

Choose a reason for hiding this comment

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

image

For self-call methods should add another activation bar to prevent confusion

<img src="UML\Images\EventListUML.png" />

And below is a sequential diagram showing a event being added, revised, checked for information and finally deleted.

Choose a reason for hiding this comment

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

image
image
image

Just some spelling errors in the sequential diagram that can be fixed for better-quality work

Copy link

@haoyangw haoyangw left a comment

Choose a reason for hiding this comment

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

Very concise and clear explanation of your features! Just some minor syntax issues with the UML diagrams. Great work! :)

@@ -0,0 +1,39 @@
@startuml
Actor -> Parser : parseAddCommand()

Choose a reason for hiding this comment

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

I think you should add a activate Parser line since the activation bar is missing from your diagram?

-> ":Storage" : loadModules()
activate ":Storage"
Alt modulesLoaded
":Storage" --> ":Storage" : :HashMap<String, NusModule>

Choose a reason for hiding this comment

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

I think this should be a solid arrow since it's a method invocation?

end
":EventListStorage" -> ":FileWriter" : taskWriter.write(gsonData)
activate ":FileWriter"
":FileWriter" --> ":File" : String

Choose a reason for hiding this comment

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

I think this should be a solid arrow since it's a method invocation?

## Getting started

Refer to the user guide [here](https://github.com/AY2223S2-CS2113-F13-3/tp/blob/master/docs/UserGuide.md).


## Design & implementation

Choose a reason for hiding this comment

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

Very clear explanation of the features! Would be helpful to have an explanation of why you implemented your feature logic this way so other developers can understand your considerations.


##### Save Events

![Save To File Sequence Diagram](UML/Images/SaveToFile.png)

Choose a reason for hiding this comment

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

Perhaps the activation bar can be a single one rather than split as the object is created once and called again later?
image


##### Load Events

![Load Events Sequence Diagram](UML/Images/loadEvents.png)

Choose a reason for hiding this comment

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

Perhaps the explanation can be more in depth to allow for a better understanding of the function?


The class diagram below illustrates the structure of the EventList component.

<img src="UML\Images\EventListUML.png" />

Choose a reason for hiding this comment

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

Perhaps the activation bar can be a single one as the object is created once and called again
image



The UML diagram below illustrates the flow of how the application adds modules:
![Add Event Class Diagram](UML/Images/addModules.png)

Choose a reason for hiding this comment

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

Perhaps you can include an activation bar as the object is created and for better visual understanding?
image

Copy link

@irving11119 irving11119 left a comment

Choose a reason for hiding this comment

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

Overall, the developer guide is easy to read and quite clear. However, some of the UML Diagrams can be further improved by following the conventions (e.g having an activation bar)

When the application starts up, the storage loadEvents() function will be called to load contents in the save file.

##### Load Modules
![Load Modules Sequence Diagram](UML/Images/LoadModules.png)

Choose a reason for hiding this comment

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

Clear and easy to read UML Diagram!

This makes life easier to developers in the future if they wish to add new features that requires users to use new commands.

### Storage Component
API: `Storage.java`

Choose a reason for hiding this comment

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

Well formatted and easy to read!

Parser -> Ui : addSuccessMsg()
Ui --> Parser
Parser --> Actor
@enduml

Choose a reason for hiding this comment

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

Perhaps you include the activation bar using activate

Choose a reason for hiding this comment

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

Additionally, remember to include the cross when you are no longer using the object/ it is deleted.

end
end
Parser --> Actor
@enduml

Choose a reason for hiding this comment

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

Similarly as above, you can include an activation bar by using activate

## Getting started

Refer to the user guide [here](https://github.com/AY2223S2-CS2113-F13-3/tp/blob/master/docs/UserGuide.md).


## Design & implementation

Choose a reason for hiding this comment

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

Perhaps you could include more elaboration such has system architecture and dependencies as well as relevant diagrams for clarity! However, it is already very clear!

activate ":Storage"
":Storage" -> ":EventListStorage" : loadEvents()
activate ":EventListStorage"
":EventListStorage" ->":EventListAdapter" : gson.fromJson(fileReader, ArrayList.class)

Choose a reason for hiding this comment

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

Is this correct? I was under the assumption that when your arrow is drawn to a class, you are calling a method in that class. Is fromJson() a method in :EventListAdapter?

## Getting started

Refer to the user guide [here](https://github.com/AY2223S2-CS2113-F13-3/tp/blob/master/docs/UserGuide.md).


## Design & implementation

Choose a reason for hiding this comment

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

I like how all the diagrams are in details and that's a whole lotta effort to do it! However it would be greater if you can have an overall architecture!


The Parser component parses the command of the user input and breaks the user input into different parts based on the flags.
This component also ensures to validate that user input is correct.

Choose a reason for hiding this comment

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

image
This sequence diagram is missing activation bars, it would be better if you include them


When the application starts up, the storage loadEvents() function will be called to load contents in the save file.
Similarly, the state of the user's event list is saved when the user exits the application by calling saveToFile().

Choose a reason for hiding this comment

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

image
I would recommend using a constructor notation like what we have learnt in this module instead of "<>" notation for creating a constructor.


And below is a sequential diagram showing a event being added, revised, checked for information and finally deleted.

<img src="UML\Images\EventListSD.png" style="zoom:80%;" />

Choose a reason for hiding this comment

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

image
there is a red dot in the middle of the diagram, take note!

kyrixn and others added 30 commits April 10, 2023 22:01
Improved specificity
Formatting
Fix Bugs
Removed typo
added on to team-based tasks
Fix bug for empty description or whitespace desription
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