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

[CS2103T-T10-4] TimeBook #54

Open
wants to merge 1,023 commits into
base: master
Choose a base branch
from

Conversation

marqueurs404
Copy link

@marqueurs404 marqueurs404 commented Sep 18, 2019

Copy link

@chrischenhui chrischenhui left a comment

Choose a reason for hiding this comment

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

For use cases:
Too little use cases.
Many gaps in user flow.
Otherwise, generally well done.

3. User chooses a free timeslot to schedule a meeting
4. TimeBook adds the scheduled meeting to all members' schedules

*Extensions*

Choose a reason for hiding this comment

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

Don't have to include extensions header if there is none.


1. User requests to arrange a meeting for a group
2. TimeBook searches for common free timeslots between all group members' schedules
3. User chooses a free timeslot to schedule a meeting

Choose a reason for hiding this comment

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

It can be confusing to the reader if you explain using the use case title.

Copy link

@chrischenhui chrischenhui left a comment

Choose a reason for hiding this comment

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

For user stories:
I think you need to at least have 30 user stories.
Otherwise, well done! 👍


|`* * *` |user |import my current schedule |do not have to manually add my calendar events

|`* * *` |user |add ad-hoc events |can de-conflict

Choose a reason for hiding this comment

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

I think you can specify/ explain what ad-hoc is supposed to mean in this context.


|`* * *` |user |export/share scheduled meetings |share it with other members of the group/project

|`* * *` |user |savable data |share it with other members of the group/project

Choose a reason for hiding this comment

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

I think this term savable data is ambiguous.

_{More to be added}_
|`* *` |careless user who type wrong commands frequently|undo my commands |do not have to manually reverse my mistakes

|`* *` |inexperienced user |group people’s timetables |complete my commands easily

Choose a reason for hiding this comment

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

Perhaps you can clarify how grouping people's timetables help to complete people's command easily.

@sreesubbash
Copy link
Member

[Feedback on Developer Guide]

3.12.2 & 3.12.2 has wrong notation for else branch and inconsistent capitalization.

3.3.1 last diagram branching does not follow if-else format. There needs to be an else case to show diagram exhausts all possibilities.

3.8.4 Activity is too nested and complex. Maybe it is better to split them and have references to the blocks.

Copy link

@gabrielseow gabrielseow left a comment

Choose a reason for hiding this comment

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

Nice DG


After the `WeekSchedule` has been generated, the method now generates a new schedule of the free timeslots within the `WeekSchedule`. It checks the `WeekSchedule` and generates a new `FreeTimeSchedule` with contains all the timeslots in which there are no clashes with the `weekSchedule`. It also only generates `FreeTimeslots` for the week.

Additionally, the generated `FreeTimeSchedule` also contains the information of the last venue the `Person` has been. This information is then used to calculate the closest location for all `Person` to meet.

Choose a reason for hiding this comment

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

Perhaps a brief description can be given to describe how you find the closest location for all of them to meet.


The following sequence diagram shows how the generateFreeTimeslot operation works:

image::FindFreeTimeslotSequenceDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

The font size is too small to be seen clearly. Perhaps you could reduce the white space to hold bigger fonts
The updateScheduleView() method is missing the activation bar. It is calling a method in the same class.
The lifeline should not continue all the way to the end with the cross on top in theory, perhaps you can have an information below stating its limitation.
the return toshowGroupCommand should be dotted line instead of the solid line
Model and SceduleGenerator missing colon
encapsulate the parameters so that people will know its a String


The following activity diagram summarizes what happens when a user interacts with the command input box:

image::command-suggestions/ActivityDiagram.svg[]
Copy link

Choose a reason for hiding this comment

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

image
No condition stated for this part.

NOTE: The vertical line/pipe character (i.e. `|`) denotes the position of the caret and is not part of the entered command itself. +
So for the above example, the command entered is `findperson n/` with the caret at the end of the command.

image::command-suggestions/Step1.svg[]
Copy link

Choose a reason for hiding this comment

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

image
The bottom of the image is faded out. The next few diagrams have the issues too


===== Aspect: How command suggestions gets its suggestions

* **Alternative 1 (current choice):** Ask ``Suggester``s for suggestions every time anything changes
Copy link

Choose a reason for hiding this comment

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

Suggesters would look better

This is the sequence diagram for the creation of the matrix.


image::gmaps/MatrixCreationSequenceDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

image

the arrow should be at the bottom

Copy link

Choose a reason for hiding this comment

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

image
The loop should be in solid lines


To support limited connectivity in our application, the results of all API queries are preprocessed and saved into the resources directory. The following activity diagram shows how the caching feature works when external data is required for the execution of a certain command:

image::gmaps/ApiDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

image
the condition should be on top and boxed up

aidilfbk and others added 30 commits November 11, 2019 23:25
Resolve developer guide issues
Change to tabular form gmaps
Switch to tabular design consideration in Developer Guide
Remove address book stuff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants