[CS2113-T11-3] MeetingJio by yanjie1017 · Pull Request #34 · nus-cs2113-AY2122S2/tp

yanjie1017 · 2022-03-09T05:07:13Z

No description provided.

shxr3f

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!

shxr3f · 2022-03-31T01:16:34Z

@@ -0,0 +1,45 @@
+@startuml


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.

shxr3f · 2022-03-31T01:22:17Z

+":DeleteCommand" -> ":Timetable" ++: remove(index - 1)
+":Timetable" --> ":DeleteCommand" ++:
+":DeleteCommand" -> ":DeleteCommand" ++: deleteConfirmation(event: Event )
+":DeleteCommand" --> ":Ui" --: deleteConfirmation(): String


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.

shxr3f · 2022-03-31T01:27:14Z

@@ -0,0 +1,29 @@
+@startuml


Misleading Usage of activation blocks. Should'nt UI still be activated after calling executeCommand() and until it recieves a return value from MeetingJio?

shxr3f · 2022-03-31T01:36:52Z

+":Ui" -> ":MeetingJio" --: executeCommand("exit", masterTimetable)
+":MeetingJio" -> ":MeetingJio" ++: exit()
+create ":StorageFile"
+":MeetingJio" -> ":StorageFile" ++: saveData(masterTimetable)


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!

shxr3f · 2022-03-31T01:50:47Z

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


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.

Teanweijun

Generally clean DG, but do synchronise across team members

Teanweijun · 2022-03-31T02:11:21Z

+`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)


Consider having more consistency in the labeling of constructor methods?

Constructor and return of Parser was clearly written but those for AddLessonCommand were omitted.

Teanweijun · 2022-03-31T02:12:55Z

+`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)


Should the lifeline of the object carry on if the class has been terminated?

Teanweijun · 2022-03-31T02:15:39Z

+![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)
+
+The following sequence diagram shows how the `add_meeting` operation works in detail.
+![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png)


Shouldn't a box in a sequence diagram be labeled as opt instead of alt if there is no else case?

Teanweijun · 2022-03-31T02:20:37Z

+![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)
+
+The following sequence diagram shows how the `add_meeting` operation works in detail.
+![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png)


Do you think that the formatting of the conditions is ambiguous?
check if meeting clash with any event and if meeting exists already could be better phrased as a boolean and add meeting does not tell me the condition for this block

Teanweijun · 2022-03-31T02:22:18Z

+![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)
+
+The following sequence diagram shows how the `add_meeting` operation works in detail.
+![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png)


Is the object for DeleteCommand necessary?

Teanweijun · 2022-03-31T02:27:26Z

+![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)
+
+The following sequence diagram shows how the `add_meeting` operation works in detail.
+![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png)


Are ERROR_OVERRLAPPING_MEETING and ERROR_DUPLICATE_MEETING methods of the UI class?

They seem to be labeled as a method invocation

Teanweijun · 2022-03-31T02:33:13Z

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


Which command is executed?

Teanweijun · 2022-03-31T02:35:10Z

+The following sequence diagram shows how the command `free` is executed.
+![FreeCommandSequenceDiagram](diagrams/FreeCommandSequenceDiagram.png)
+
+## 3.4 Delete events `delete`


Is the additional delete in this line intentional? Seems inconsistent compared to the rest of the document

Teanweijun · 2022-03-31T02:38:24Z

+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)


Diagram formatting is inconsistent. Consider deciding whether to include the bottom row of boxes or not

Teanweijun · 2022-03-31T02:42:49Z

+
+Example of usage:
+
+`delete n/John i/1`


Perhaps there is a lack of example inputs/outputs? Consider adding this for your add portion as well

thernturnaround

Overall readable and understandable documentation.

Could use more standardisation across sentence structure/phrasing/diagrams.

thernturnaround · 2022-04-01T02:24:49Z

+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)


Could you consider splitting the different steps in Storage into multiple sequence diagrams?

Use ref frames to break up the content, e.g.

The bottom part (where you load meetings) could be a ref frame, which you can elaborate on later

Since theres only one condition, can the 2nd alt be replaced by opt?

thernturnaround · 2022-04-01T02:32:25Z

+encountered along the way, an error message will be shown.
+
+The following sequence diagram shows how the save operation works:
+![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.png)


Should the lifeline extend past the point where an object is deleted?

thernturnaround · 2022-04-01T02:33:49Z

+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)


Is it possible to use ref to break up the diagrams into smaller pieces?

thernturnaround · 2022-04-01T02:35:49Z

+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)


Can the footer be removed?

hide footbox

Consider adding in object deletion notations where appropriate

thernturnaround · 2022-04-01T02:39:56Z

+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)


(Not too sure)
Can you remove the return types when constructing an object? I think you can omit return types when calling constructors.

thernturnaround · 2022-04-01T02:41:55Z

+`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)


Consider using

[ -> Command instead of none -> Command.

This can help you remove the "Unspecified" object.

ElaineQT

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!

ElaineQT · 2022-04-01T02:08:51Z

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


Should this be updated?

ElaineQT · 2022-04-01T02:10:59Z

+- Timetables
+- Events
+
+## 2.5 Storage Component


Perhaps StorageFile class can be mentioned here.

ElaineQT · 2022-04-01T02:16:38Z

+
+The following sequence diagram shows how the `add` operation generally works.
+
+![AddCommandSequenceDiagram](diagrams/AddCommandSequenceDiagram.png)


Should parseCommand() call be a solid line?

Perhaps the deletion cross should end the life line, rather than the activation bar?

E.g.

ElaineQT · 2022-04-01T02:18:32Z

+Common Term | User-specific term | Lesson-specific term | Meeting-specific term      |
+------------|-------------------|----------------------|----------------------------|        
+add | `"add_user ..."`  | `"add_lesson ..."`   | `"add_meeting ..."`     
+AddCommand | `AddUserCommand`  | `AddLessonCommand`   | `AddMeetingCommand`  


This annotaion is very clear and rigorous. Good job!

ElaineQT · 2022-04-01T02:25:15Z

+>To be added
+
+The following sequence diagram shows how the `add_meeting` operation works in detail.
+![AddMeetingCommandSequence](diagrams/AddMeetingCommandSequence.png)


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.

ElaineQT · 2022-04-01T02:33:53Z

+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)


How does the control flow go from 1 to 2? Or should the activation bar end at 1?

ElaineQT · 2022-04-01T02:36:56Z

+encountered along the way, an error message will be shown.
+
+The following sequence diagram shows how the save operation works:
+![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.png)


Perhaps the message should start from the activation bar?

ElaineQT · 2022-04-01T02:41:15Z

+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)


Should there be an activation bar after object creation?

wli-linda

Overall clear sequence diagrams. Just a few suggestions on style, but good work!

wli-linda · 2022-04-01T02:47:10Z

+create Parser
+Ui -> Parser : Parser("add...")
+activate Parser
+Ui <-- Parser : parser :Parser


Maybe you could omit the description here, since it's clear that a Parser object is being constructed? But either way works.

wli-linda · 2022-04-01T02:48:44Z

+Parser <-- AddCommand
+deactivate AddCommand
+
+Parser --> Parser : :Command


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.

wli-linda · 2022-04-01T02:50:52Z

+":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)


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?

wli-linda · 2022-04-01T02:52:34Z

+":MasterTimetable" -> ":MasterTimetable" ++: deleteMeetingFromTimetable(timetable:Timetable,meeting:Meeting)
+":MasterTimetable" -> ":MasterTimetable" --: deleteMeetingFromMeetingList(meeting:Meeting)
+":MasterTimetable" -> ":DeleteCommand" --: deleteMeetingFromAllTimetableConfirmation(meeting:Meeting)


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?

…ore 0800

…for free command

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]'

…r first

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

shxr3f reviewed Mar 31, 2022

View reviewed changes

Teanweijun reviewed Mar 31, 2022

View reviewed changes

thernturnaround reviewed Apr 1, 2022

View reviewed changes

ElaineQT reviewed Apr 1, 2022

View reviewed changes

wli-linda reviewed Apr 1, 2022

View reviewed changes

angyongming and others added 25 commits April 6, 2022 16:13

Handle exception when free command is used with an event starting bef…

e06aa8b

…ore 0800

Modify JavaDoc to take into account changes to free command

a22fae6

Revamp free command to take into account 0000-0800

caab62d

Update exception handling, error messages, javadoc and JUnit testing …

808b07e

…for free command

Minor edits to free command and update user guide

9f6d1f5

Modify according to TA's code review

d3f3eee

Merge pull request #220 from angyongming/free-error-catching-branch

170d290

Rectify Free Command

Edit error message for user does not exist to suggest user to add_use…

6130c8e

…r first

Update user guide and developer guide

6b40637

Do up PPP

36551a6

Merge pull request #221 from angyongming/ppp-branch

878a500

Fill up ppp

Fix minor issues and checkstyle

8708046

Add checkName

48431d4

Format error messages

2fda9a5

Make meetings non-editable

5e4c8f5

Fix bug in prepareEdit

c95ac26

Fix checkstyle

c403530

Fix checkstyle

b7ba033

Fix checkstyle

84d452e

Fix checkstyle

760d2b8

Fix checkstyle

f5efa3e

Fix checkstyle

e1aa56b

Fix checkstyle

1c57d14

Fix checktstyle

157b2f5

DESKTOP-91KNFFF\LI HAO and others added 30 commits April 11, 2022 16:52

make changes to class diagrams and user guide

a96265f

Merge branch 'master' of https://github.com/AY2122S2-CS2113-T11-3/tp

d0c580a

Change DG

8965e3c

Merge pull request #271 from lihao-InfoSec/master

1a0c0fc

fix styling

Edit the remaining changes

9e26550

system.exit

Merge branch 'master' of https://github.com/angyongming/tp

f475ec3

* '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

Merge pull request #273 from yanjie1017/master

feec4a7

Change DG

rectify formatting error in ppp

9a41220

Merge pull request #275 from angyongming/ppp-format-branch

362ec6b

rectify ppp formatting error

Merge pull request #274 from ibrahimisramos/Minor-edits

9fb32d8

Edit the remaining changes

Edit sq for delete command activation bar

3476f7c

Merge branch 'master' of https://github.com/angyongming/tp

a0008ec

* 'master' of https://github.com/angyongming/tp: rectify formatting error in ppp Edit the remaining changes system.exit Change DG

Add photo

863f873

Edit ppp

281a46b

Merge pull request #276 from ibrahimisramos/Minor-edits

bf1eae7

Edit sq for delete command activation bar

Merge pull request #277 from angyongming/photo-branch

a43de92

Add photo and edit PPP

Change sequence diagram

55244e6

Merge pull request #278 from yanjie1017/master

c239e6a

Fix sequence diagram

Edit sequence diagrams

71a5550

Format link in DG

40e6a55

Edit load and save sequence diagram

e92392e

Merge branch 'master' of https://github.com/AY2122S2-CS2113-T11-3/tp

9a61d10

Merge branch 'master' into new-changes-sq

fa399c5

Merge pull request #281 from lihao-InfoSec/master

de19a9a

Edit save and load sequence diagram

Merge pull request #280 from yanjie1017/master

0a3ccc7

Format link in DG

Merge pull request #279 from ibrahimisramos/new-changes-sq

14c3c43

Edit sequence diagrams

update user guide and load sequence diagram

2076b34

Merge branch 'master' of https://github.com/AY2122S2-CS2113-T11-3/tp

4e862f1

Merge pull request #282 from lihao-InfoSec/master

5872042

update user guide and load sequence diagram


		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.

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


		The following sequence diagram shows how the `add` operation generally works.

		![AddCommandSequenceDiagram](diagrams/AddCommandSequenceDiagram.png)

Uh oh!

Conversation

yanjie1017 commented Mar 9, 2022

Uh oh!

shxr3f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Teanweijun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thernturnaround left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElaineQT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!