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-T12-3] Student Exchange Programme Helper #22

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

Conversation

L0Z1K
Copy link

@L0Z1K L0Z1K commented Mar 2, 2023

Preparing NUS Mech Eng students to embark on a SEP, as journeying into a foreign country is overwhelming. Our solution will be a quick way to let them know which modules can be mapped to their desired university and the better flight and accommodation options that are within their budget.

Sequence Diagram of Storage initialisation:

![Storage.png](diagrams%2FStorage%2FStorage.png)

Choose a reason for hiding this comment

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

Image looks blur on the website

Choose a reason for hiding this comment

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

agree

# Architecture

![Architecture.png](diagrams%2FArchitecture.png)

Choose a reason for hiding this comment

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

Architecture is clear and easy to understand.

The following sequence diagram shows the relationship between the classes involved when the delete command is called.

![HelpCommandSequenceDiagram.png](diagrams%2FHelpCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

Activation bar of :HelpCommandModule appears to be cut off from the top and bottom

Copy link

@Jeraldchen Jeraldchen left a comment

Choose a reason for hiding this comment

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

Overall DG is clear and well done. Just some minor errors to look into as shown in comments

Copy link

@wanjuin wanjuin left a comment

Choose a reason for hiding this comment

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

goos use of sequence diagram (UML) in the DG overall.

Copy link

@BenjaminPoh BenjaminPoh left a comment

Choose a reason for hiding this comment

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

Overall good job, explanations were clear and diagrams were easy to understand!

2. Parser : Processes and Executes User Commands
3. UI : Prints out messages to user
4. Storage : Processes and stores
5. DataReader

Choose a reason for hiding this comment

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

Is there a missing whitespace here? (Diagram says "Data Reader")


The following class diagrams illustrates the relationship between the Parser class and the Command classes.
- (TODO: finish up the rest of the command cases)
![ParserSequenceDiagram.png](diagrams%2FParserSequenceDiagram.png)

Choose a reason for hiding this comment

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

  1. The UML Diagram is very large, and many of the command classes are still incomplete. Maybe try to break it up into different diagrams or use reference frames

  2. There is also a case of 4-level self-invocation. Maybe can also make use of reference frames for this?


The following sequence diagram shows the relationship between the classes involved when the delete command is called.

![DeleteModuleCommandSequenceDiagram.png](diagrams%2FDeleteModuleCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

For return values, I think it is sufficient to write "true" instead of "return true", as it should already be clear it is a return value by the dotted arrow.


Sequence Diagram of List Current Command.

![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

Is there a reason why the dotted arrow at the end of ui:UI goes leftwards instead of right like other arrows?

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

missing information?


DataReader class reads two external .txt files to acquire the list of Partner Universities and list
of Modules available in the PUs, and provides this information to other components.

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}

Choose a reason for hiding this comment

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

the uml diagram is missing

Sequence Diagram of Storage initialisation:

![Storage.png](diagrams%2FStorage%2FStorage.png)

Choose a reason for hiding this comment

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

agree


Sequence Diagram of List Current Command.

![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

image

arrow is on the wrong side

> Syntax: list current [_uniAbbreviation_]

Sequence Diagram of List Current Pu Command
![ListCurrentPuCommandSequenceDiagram.png](diagrams%2FListCurrentPuCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

image

missing activation bars


The following sequence diagram shows the relationship between the classes involved when the delete command is called.

![DeleteModuleCommandSequenceDiagram.png](diagrams%2FDeleteModuleCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

image

activation bar should end when the method ends

The help command provides a list of commands and the commands' respective input format for the user.
> Syntax: /help

The following sequence diagram shows the relationship between the classes involved when the delete command is called.
Copy link

Choose a reason for hiding this comment

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

There is a typo here regarding the delete command


The following sequence diagram shows the relationship between the classes involved when the delete command is called.

![HelpCommandSequenceDiagram.png](diagrams%2FHelpCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

The method call printHelpCommandMessage() from :HelpModuleCommand should be at the start of the activation bar.

**Note: Partner Universities Abbreviations can be found using List Pu command**

Sequence Diagram of List Pu Modules Command.
![ListPuModulesCommandSequenceDiagram.png](diagrams%2FListPuModulesCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Nice diagram! But the final return from the static UI class should be at the end of the activation bar


Sequence Diagram of List Current Command.

![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Nice! It would be better if the self-invoked method had an return arrow on the same side of the activation bar going back into the bar.


Sequence Diagram of Storage initialisation:

![Storage.png](diagrams%2FStorage%2FStorage.png)
Copy link

Choose a reason for hiding this comment

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

It would be better to add figure numbers to your diagrams too!

Copy link

@itszhixuan itszhixuan left a comment

Choose a reason for hiding this comment

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

Generally well done, except for some small mistakes.

Comment on lines 92 to 94
Sequence Diagram of Storage initialisation:

![Storage.png](diagrams%2FStorage%2FStorage.png)

Choose a reason for hiding this comment

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

Sequence diagram looks blur, but it seems like the return values are not indicated with a dotted line.

Comment on lines 106 to 109
The commands that the parser class will initialise are ListPuCommand(), ListCurrentCommand(modules),
prepareListPuModulesCommand(userCommandSecondKeyword, universities), ExitCommand(),
prepareAddModuleCommand(storage, userCommandSecondKeyword, puModules, universities),
DeleteModuleCommand(storage, indexToRemove, modules), and HelpCommand(). The parser class will handle error checking by

Choose a reason for hiding this comment

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

Perhaps you can consider reformatting this to make it easier to read, as all the commands are lumped together and messy.


The following class diagrams illustrates the relationship between the Parser class and the Command classes.
- (TODO: finish up the rest of the command cases)
![ParserSequenceDiagram.png](diagrams%2FParserSequenceDiagram.png)

Choose a reason for hiding this comment

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

The third alt case does not state the condition explicitly, which makes it unclear on when this is called.


### Add Module Command

Adds the Module the user has wants to save to the saved modules database.

Choose a reason for hiding this comment

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

Is this line correct? I am still able to understand but seems like there is some typo.

> Syntax: list current [_uniAbbreviation_]

Sequence Diagram of List Current Pu Command
![ListCurrentPuCommandSequenceDiagram.png](diagrams%2FListCurrentPuCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

Another small typo in the image "UserConcole"

**Note: Partner Universities Abbreviations can be found using List Pu command**

Sequence Diagram of List Pu Modules Command.
![ListPuModulesCommandSequenceDiagram.png](diagrams%2FListPuModulesCommandSequenceDiagram.png)

Choose a reason for hiding this comment

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

I feel like your UML diagram is a bit big and filled with a lot of words. Maybe you could try to make it more concise by making it small and precise.

briantjs00 and others added 20 commits April 6, 2023 00:51
…into Branch-DeadlineStorage-Singleton

# Conflicts:
#	src/main/java/seedu/duke/Duke.java
#	src/test/java/seedu/duke/ParserTest.java
#	src/test/java/seedu/duke/command/DeleteModuleCommandTest.java
#	src/test/java/seedu/duke/command/ExitCommandTest.java
#	src/test/java/seedu/duke/command/HelpCommandTest.java
#	src/test/java/seedu/duke/command/InvalidCommandTest.java
Change DeadlineStorage classs into singleton pattern
…into branch-Parser-Singleton

# Conflicts:
#	src/test/java/seedu/duke/command/DeleteModuleCommandTest.java
#	src/test/java/seedu/duke/command/ExitCommandTest.java
#	src/test/java/seedu/duke/command/HelpCommandTest.java
#	src/test/java/seedu/duke/command/InvalidCommandTest.java
Change Parser class into singleton pattern
SSzeWen and others added 30 commits April 10, 2023 21:02
Update DG Deadline and Budget test details
Update Developer Guide Budget Commands
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.