-
Notifications
You must be signed in to change notification settings - Fork 248
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
base: master
Are you sure you want to change the base?
Conversation
Sequence Diagram of Storage initialisation: | ||
|
||
![Storage.png](diagrams%2FStorage%2FStorage.png) | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) | ||
|
There was a problem hiding this comment.
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) | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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!
docs/DeveloperGuide.md
Outdated
2. Parser : Processes and Executes User Commands | ||
3. UI : Prints out messages to user | ||
4. Storage : Processes and stores | ||
5. DataReader |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
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
-
There is also a case of 4-level self-invocation. Maybe can also make use of reference frames for this?
docs/DeveloperGuide.md
Outdated
|
||
The following sequence diagram shows the relationship between the classes involved when the delete command is called. | ||
|
||
![DeleteModuleCommandSequenceDiagram.png](diagrams%2FDeleteModuleCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
|
||
Sequence Diagram of List Current Command. | ||
|
||
![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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?
docs/DeveloperGuide.md
Outdated
## 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing information?
docs/DeveloperGuide.md
Outdated
|
||
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.} |
There was a problem hiding this comment.
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) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
docs/DeveloperGuide.md
Outdated
|
||
Sequence Diagram of List Current Command. | ||
|
||
![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
> Syntax: list current [_uniAbbreviation_] | ||
|
||
Sequence Diagram of List Current Pu Command | ||
![ListCurrentPuCommandSequenceDiagram.png](diagrams%2FListCurrentPuCommandSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
|
||
The following sequence diagram shows the relationship between the classes involved when the delete command is called. | ||
|
||
![DeleteModuleCommandSequenceDiagram.png](diagrams%2FDeleteModuleCommandSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/DeveloperGuide.md
Outdated
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. |
There was a problem hiding this comment.
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
docs/DeveloperGuide.md
Outdated
|
||
The following sequence diagram shows the relationship between the classes involved when the delete command is called. | ||
|
||
![HelpCommandSequenceDiagram.png](diagrams%2FHelpCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
**Note: Partner Universities Abbreviations can be found using List Pu command** | ||
|
||
Sequence Diagram of List Pu Modules Command. | ||
![ListPuModulesCommandSequenceDiagram.png](diagrams%2FListPuModulesCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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
docs/DeveloperGuide.md
Outdated
|
||
Sequence Diagram of List Current Command. | ||
|
||
![ListCurrentCommandSequenceDiagram.png](diagrams%2FListCurrentCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
|
||
Sequence Diagram of Storage initialisation: | ||
|
||
![Storage.png](diagrams%2FStorage%2FStorage.png) |
There was a problem hiding this comment.
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!
There was a problem hiding this 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.
docs/DeveloperGuide.md
Outdated
Sequence Diagram of Storage initialisation: | ||
|
||
![Storage.png](diagrams%2FStorage%2FStorage.png) |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
|
||
### Add Module Command | ||
|
||
Adds the Module the user has wants to save to the saved modules database. |
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
> Syntax: list current [_uniAbbreviation_] | ||
|
||
Sequence Diagram of List Current Pu Command | ||
![ListCurrentPuCommandSequenceDiagram.png](diagrams%2FListCurrentPuCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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"
docs/DeveloperGuide.md
Outdated
**Note: Partner Universities Abbreviations can be found using List Pu command** | ||
|
||
Sequence Diagram of List Pu Modules Command. | ||
![ListPuModulesCommandSequenceDiagram.png](diagrams%2FListPuModulesCommandSequenceDiagram.png) |
There was a problem hiding this comment.
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.
…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
Update DG Deadline and Budget test details
Fix bugs in DG
Update parser sequence diagrams
Update PPP
Update AboutUs
Update Developer Guide Budget Commands
Format budget DG
Change UG DG QUICK START
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.