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-T11-3] MeetingJio #34

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

Conversation

yanjie1017
Copy link

No description provided.

Copy link

@shxr3f shxr3f left a 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
Copy link

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.

Copy link

Choose a reason for hiding this comment

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

image

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

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.

Copy link

Choose a reason for hiding this comment

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

image

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

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?

Copy link

Choose a reason for hiding this comment

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

image

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

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!

Copy link

Choose a reason for hiding this comment

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

image

Comment on lines 25 to 36
## 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
Copy link

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.

Copy link

Choose a reason for hiding this comment

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

image

Copy link

@Teanweijun Teanweijun left a 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

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

Choose a reason for hiding this comment

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

Consider having more consistency in the labeling of constructor methods?
image
Constructor and return of Parser was clearly written but those for AddLessonCommand were omitted.

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

Choose a reason for hiding this comment

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

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

![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)

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

Choose a reason for hiding this comment

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

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

![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)

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

Choose a reason for hiding this comment

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

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
image
image

![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)

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

Choose a reason for hiding this comment

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

Is the object for DeleteCommand necessary?
image

![AddLessonSequenceDiagram](diagrams/AddLessonSequenceDiagram.png)

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

Choose a reason for hiding this comment

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

Are ERROR_OVERRLAPPING_MEETING and ERROR_DUPLICATE_MEETING methods of the UI class?
image
They seem to be labeled as a method invocation


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.

Choose a reason for hiding this comment

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

Which command is executed?

The following sequence diagram shows how the command `free` is executed.
![FreeCommandSequenceDiagram](diagrams/FreeCommandSequenceDiagram.png)

## 3.4 Delete events `delete`

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

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)

Choose a reason for hiding this comment

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

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


Example of usage:

`delete n/John i/1`

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

Copy link

@thernturnaround thernturnaround left a 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.

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)

Choose a reason for hiding this comment

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

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

image

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?

encountered along the way, an error message will be shown.

The following sequence diagram shows how the save operation works:
![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.png)

Choose a reason for hiding this comment

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

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

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)

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?

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)

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

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

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)

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Consider using

[ -> Command instead of none -> Command.

image

This can help you remove the "Unspecified" object.

Copy link

@ElaineQT ElaineQT left a 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!

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

Choose a reason for hiding this comment

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

Should this be updated?

- Timetables
- Events

## 2.5 Storage Component
Copy link

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.


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

![AddCommandSequenceDiagram](diagrams/AddCommandSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Should parseCommand() call be a solid line?
image
Perhaps the deletion cross should end the life line, rather than the activation bar?
image
E.g.
image

Comment on lines 44 to 47
Common Term | User-specific term | Lesson-specific term | Meeting-specific term |
------------|-------------------|----------------------|----------------------------|
add | `"add_user ..."` | `"add_lesson ..."` | `"add_meeting ..."`
AddCommand | `AddUserCommand` | `AddLessonCommand` | `AddMeetingCommand`
Copy link

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!

>To be added

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

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.

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)
Copy link

Choose a reason for hiding this comment

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

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

encountered along the way, an error message will be shown.

The following sequence diagram shows how the save operation works:
![SaveSequenceDiagram](diagrams/SaveSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Perhaps the message should start from the activation bar?
image

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)
Copy link

Choose a reason for hiding this comment

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

Should there be an activation bar after object creation?
image

Copy link

@wli-linda wli-linda left a 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

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

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)

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?

Comment on lines 19 to 21
":MasterTimetable" -> ":MasterTimetable" ++: deleteMeetingFromTimetable(timetable:Timetable,meeting:Meeting)
":MasterTimetable" -> ":MasterTimetable" --: deleteMeetingFromMeetingList(meeting:Meeting)
":MasterTimetable" -> ":DeleteCommand" --: deleteMeetingFromAllTimetableConfirmation(meeting:Meeting)

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?

angyongming and others added 25 commits April 6, 2022 16:13
* '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]'
DESKTOP-91KNFFF\LI HAO and others added 30 commits April 11, 2022 16:52
* '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
* '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
* '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
update user guide and load sequence diagram
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.

8 participants