-
Notifications
You must be signed in to change notification settings - Fork 101
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-T11-3] MeetingJio #34
base: master
Are you sure you want to change the base?
[CS2113-T11-3] MeetingJio #34
Conversation
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 seems like a pretty neat and user friendly Developer Guide. However there are some issues present in the sequence diagrams. Could also use some detailed descriptions in areas. The use of class diagrams could also help in reading the sequence diagrams better, as it will show all the methods a particular class has and the variables it holds. Good work so far just some minor improvements to make it even better!! All the best!
@@ -0,0 +1,45 @@ | |||
@startuml |
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.
Diagram looks very small on the site itself. Although zooming in does help with deciphering what is written in the diagram. Could be better by using SD in sequence diagrams so as to shift so parts of the diagram to another picture to make it more visible without the need to zoom.
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.
":DeleteCommand" -> ":Timetable" ++: remove(index - 1) | ||
":Timetable" --> ":DeleteCommand" ++: | ||
":DeleteCommand" -> ":DeleteCommand" ++: deleteConfirmation(event: Event ) | ||
":DeleteCommand" --> ":Ui" --: deleteConfirmation(): String |
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 deleteConfirmation() a method call from the Command object on the UI? If its a method call why is dotted line being used? It can be confusing what you are trying to show here.
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.
@@ -0,0 +1,29 @@ | |||
@startuml |
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.
Misleading Usage of activation blocks. Should'nt UI still be activated after calling executeCommand() and until it recieves a return value from MeetingJio?
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.
":Ui" -> ":MeetingJio" --: executeCommand("exit", masterTimetable) | ||
":MeetingJio" -> ":MeetingJio" ++: exit() | ||
create ":StorageFile" | ||
":MeetingJio" -> ":StorageFile" ++: saveData(masterTimetable) |
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 the saveData() method instantiating a Storage object here since its pointing to the initial object? This is also confusing as usually the constructor method instantiates the object. Method calls should be pointing to the activation bar instead!
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
## 3.1 Add Feature | ||
`add_lesson` allows users to create a new lesson and add it to their own timetables | ||
|
||
`add_meeting` allows users to create a new meeting and add it to all the existing timetables | ||
|
||
The following sequence diagram shows how the `add_lesson` operation generally works. | ||
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png) | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png) | ||
|
||
## 3.2 Listing Events Feature |
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 simple and straightforward language used in descriptions hence easy to read and understand. However some descriptions could have been more descriptive (Ie explaining what the sequence diagrams show) this would help in reducing misconceptions that could be made by reader.
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 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 clean DG, but do synchronise across team members
docs/DeveloperGuide.md
Outdated
`add_meeting` allows users to create a new meeting and add it to all the existing timetables | ||
|
||
The following sequence diagram shows how the `add_lesson` operation generally works. | ||
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.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
`add_meeting` allows users to create a new meeting and add it to all the existing timetables | ||
|
||
The following sequence diagram shows how the `add_lesson` operation generally works. | ||
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.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
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png) | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.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
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png) | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.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
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png) | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.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
![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png) | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.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
|
||
Before the timetable is listed out, it will also be sorted according to day and time for easy reading. | ||
|
||
The following sequence diagram shows how the command `` is executed. |
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.
Which command is executed?
docs/DeveloperGuide.md
Outdated
The following sequence diagram shows how the command `free` is executed. | ||
![FreeCommandSequenceDiagram](diagrams/FreeCommandSequenceDiagram.png) | ||
|
||
## 3.4 Delete events `delete` |
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 the additional delete
in this line intentional? Seems inconsistent compared to the rest of the document
docs/DeveloperGuide.md
Outdated
Before the timetable is listed out, it will also be sorted according to day and time for easy reading. | ||
|
||
The following sequence diagram shows how the command `` is executed. | ||
![ListCommandSequenceDiagram](diagrams/ListCommandSequenceDiagram.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.
|
||
Example of usage: | ||
|
||
`delete n/John i/1` |
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 there is a lack of example inputs/outputs? Consider adding this for your add
portion as well
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 readable and understandable documentation.
Could use more standardisation across sentence structure/phrasing/diagrams.
docs/DeveloperGuide.md
Outdated
If yes, it will call for ParserLocalData.prepareLoadMeeting method to add all the meetings into the list. | ||
|
||
The following sequence diagram shows how the load operation works: | ||
![LoadSequenceDiagram](diagrams/LoadSequenceDiagram.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.
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.
Since theres only one condition, can the 2nd alt be replaced by opt?
docs/DeveloperGuide.md
Outdated
encountered along the way, an error message will be shown. | ||
|
||
The following sequence diagram shows how the save operation works: | ||
![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.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
If the event is a meeting, it will delete this event from all users | ||
|
||
The following sequence diagram shows how the `delete` command works: | ||
![DeleteCommandSequenceDiagram](diagrams/DeleteCommandSequenceDiagram.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 it possible to use ref to break up the diagrams into smaller pieces?
docs/DeveloperGuide.md
Outdated
Timings that are not marked 'busy' are then identified as free time slots. | ||
|
||
The following sequence diagram shows how the command `free` is executed. | ||
![FreeCommandSequenceDiagram](diagrams/FreeCommandSequenceDiagram.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.
Can the footer be removed?
hide footbox
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.
Consider adding in object deletion notations where appropriate
docs/DeveloperGuide.md
Outdated
Before the timetable is listed out, it will also be sorted according to day and time for easy reading. | ||
|
||
The following sequence diagram shows how the command `` is executed. | ||
![ListCommandSequenceDiagram](diagrams/ListCommandSequenceDiagram.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
`add_user` adds new user and his or her timetable into the master timetable. | ||
|
||
The following sequence diagram shows how the `add_user` command works: | ||
![AddUserCommandSequenceDiagram](diagrams/AddUserCommandSequenceDiagram.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.
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.
Although there might be some minor issues in the diagrams, they are generally well done. The developer guide is very comprehensive and well-organized. The explanation and annotation are also clear. Good job!
docs/DeveloperGuide.md
Outdated
@@ -4,26 +4,207 @@ | |||
|
|||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} |
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.
Should this be updated?
docs/DeveloperGuide.md
Outdated
- Timetables | ||
- Events | ||
|
||
## 2.5 Storage Component |
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 StorageFile class can be mentioned here.
docs/DeveloperGuide.md
Outdated
|
||
The following sequence diagram shows how the `add` operation generally works. | ||
|
||
![AddCommandSequenceDiagram](diagrams/AddCommandSequenceDiagram.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
Common Term | User-specific term | Lesson-specific term | Meeting-specific term | | ||
------------|-------------------|----------------------|----------------------------| | ||
add | `"add_user ..."` | `"add_lesson ..."` | `"add_meeting ..."` | ||
AddCommand | `AddUserCommand` | `AddLessonCommand` | `AddMeetingCommand` |
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.
This annotaion is very clear and rigorous. Good job!
docs/DeveloperGuide.md
Outdated
>To be added | ||
|
||
The following sequence diagram shows how the `add_meeting` operation works in detail. | ||
![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.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.
If the checkIfClash() method is always called, perhaps it should not be inside the opt frame. Rather, the guard condition can be "[If clash]", then only the returning error message part is inside the opt frame.
Similarly for the checkIfMeetingExistsAready() part.
docs/DeveloperGuide.md
Outdated
If the event is a meeting, it will delete this event from all users | ||
|
||
The following sequence diagram shows how the `delete` command works: | ||
![DeleteCommandSequenceDiagram](diagrams/DeleteCommandSequenceDiagram.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
encountered along the way, an error message will be shown. | ||
|
||
The following sequence diagram shows how the save operation works: | ||
![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.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
If yes, it will call for ParserLocalData.prepareLoadMeeting method to add all the meetings into the list. | ||
|
||
The following sequence diagram shows how the load operation works: | ||
![LoadSequenceDiagram](diagrams/LoadSequenceDiagram.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.
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 clear sequence diagrams. Just a few suggestions on style, but good work!
create Parser | ||
Ui -> Parser : Parser("add...") | ||
activate Parser | ||
Ui <-- Parser : parser :Parser |
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.
Maybe you could omit the description here, since it's clear that a Parser object is being constructed? But either way works.
Parser <-- AddCommand | ||
deactivate AddCommand | ||
|
||
Parser --> Parser : :Command |
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.
Maybe it would be good to omit this return line on a self-invocation also? I'm not sure if it conveys any extra information.
":Parser" --> ":Ui" --: parser:Parser | ||
":Ui" -> ":Parser" ++: parseCommand("add_meeting t/meeting1 d/Friday st/1230 et/1330 m/online") | ||
create ":AddMeetingCommand" | ||
":Parser" -> ":AddMeetingCommand" ++: new AddMeetingCommand("meeting1": String,"Friday":String,1230:int,1330:int,"online":String) |
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.
Would like to echo above comment that the font size looks a bit small. Consider using more descriptive statements here instead of writing the whole function call, as the function call is very long and stretches the diagram horizontally?
":MasterTimetable" -> ":MasterTimetable" ++: deleteMeetingFromTimetable(timetable:Timetable,meeting:Meeting) | ||
":MasterTimetable" -> ":MasterTimetable" --: deleteMeetingFromMeetingList(meeting:Meeting) | ||
":MasterTimetable" -> ":DeleteCommand" --: deleteMeetingFromAllTimetableConfirmation(meeting:Meeting) |
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.
Again, maybe consider describing these calls in words instead of writing the entire function call, so that the diagram is less stretched horizontally and more readable?
Rectify Free Command
* 'master' of https://github.com/angyongming/tp: Modify according to TA's code review Minor edits to free command and update user guide Update exception handling, error messages, javadoc and JUnit testing for free command Revamp free command to take into account 0000-0800 Modify JavaDoc to take into account changes to free command Handle exception when free command is used with an event starting before 0800 Catch exception when user inputs 'free [negative number]'
Fill up ppp
fix styling
system.exit
* 'master' of https://github.com/angyongming/tp: make changes to class diagrams and user guide Edit free error message Edit ppp space fix styling Change error meggaes fix styling fix styling
Change DG
rectify ppp formatting error
Edit the remaining changes
* 'master' of https://github.com/angyongming/tp: rectify formatting error in ppp Edit the remaining changes system.exit Change DG
Edit sq for delete command activation bar
Add photo and edit PPP
Fix sequence diagram
* 'master' of https://github.com/ibrahimisramos/tp: (22 commits) Change sequence diagram Edit ppp Add photo Edit sq for delete command activation bar rectify formatting error in ppp Edit the remaining changes system.exit Change DG make changes to class diagrams and user guide Edit free error message Edit ppp space fix styling Change error meggaes fix styling Edit DG to standardise and remove architecture fix styling Add in Ctrl-C case in UG under limitations Add case where ctrl c is placed, print error message, everything cleared change naming conventions for junit testing for list and free command Update PP Change naming of clearcommand edit timetable class diagram to include meeting list edit dg ... # Conflicts: # docs/diagrams/DeleteCommandSequenceDiagram.puml # docs/images/AddMeetingCommandSequence.png # docs/images/DeleteCommandSequenceDiagram.png
Edit save and load sequence diagram
Format link in DG
Edit sequence diagrams
update user guide and load sequence diagram
No description provided.