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-W16-3] StudyBuddy #55

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

Conversation

teikjun
Copy link

@teikjun teikjun commented Feb 26, 2020

The pull request for Team W16-3

@@ -297,33 +297,32 @@ Priorities: High (must have) - `* * \*`, Medium (nice to have) - `* \*`, Low (un
|Priority |As a ... |I want to ... |So that I can...
|`* * *` |new user |see usage instructions |refer to instructions when I forget how to use the App

|`* * *` |user |add a new person |
|`* * *` |user |add a new task |

Choose a reason for hiding this comment

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

By removing persons object from your project, does this mean that there's no longer a address book functionality at all?

@@ -347,7 +346,7 @@ _{More to be added}_
== Non Functional Requirements

. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `11` or above installed.
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. Should be able to hold up to 1000 tasks without a noticeable sluggishness in performance for typical usage.

Choose a reason for hiding this comment

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

noticeable sluggishness should be quantified (e.g 300ms or smth)

@@ -297,33 +297,32 @@ Priorities: High (must have) - `* * \*`, Medium (nice to have) - `* \*`, Low (un
|Priority |As a ... |I want to ... |So that I can...
|`* * *` |new user |see usage instructions |refer to instructions when I forget how to use the App

Choose a reason for hiding this comment

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

Can filtering searches be added as a user story?

@@ -347,7 +346,7 @@ _{More to be added}_
== Non Functional Requirements

Choose a reason for hiding this comment

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

Any more NFRs to add? e.g. should be NUS student, response time of system

@@ -347,7 +346,7 @@ _{More to be added}_
== Non Functional Requirements

. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `11` or above installed.
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. Should be able to hold up to 1000 tasks without a noticeable sluggishness in performance for typical usage.
Copy link

Choose a reason for hiding this comment

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

need to be more specific about what is a noticeable sluggishness

@okkhoy
Copy link
Contributor

okkhoy commented Mar 23, 2020

Please retitle the PR with the correct format.

@teikjun teikjun changed the title Pull request for Team W16-3 [CS2103T-W16-3] StudyBuddy Mar 23, 2020
@suwoons
Copy link

suwoons commented Mar 25, 2020

Section 3.1.2

  • Small typo in "list of tasks due soon will b e initialized"

Section 3.2.1

  • Small typo in "displays a calendar fo users"

Generally clear and understandable, but perhaps having at least one class diagram for your modules such as Calendar or the Due Soon Task feature would enhance the comprehensiveness of your DG :)

Copy link

@wxwxwxwx9 wxwxwxwx9 left a comment

Choose a reason for hiding this comment

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

Overall, there were some flaws in the UML diagrams.

@@ -98,39 +123,34 @@ image::LogicClassDiagram.png[]
*API* :

Choose a reason for hiding this comment

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

For the logic component diagram, it seems like a lot of things were changed (e.g. ArgumentMultimap, Tokenizer) but from what I've seen from the codebase, those classes seems to be still there. Is this intended?

Also, there is no interaction between the logic class and parser class, which is a little weird because typically they are related to one another.


NOTE: The lifeline for `UndoCommand` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram.
image::DueSoonSequenceDiagram.png[width = "600", length = "500"]

Choose a reason for hiding this comment

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

  1. Is the group still using AddressBook model? If not, remember to remove it / rename it appropriately from your sequence diagram!

  2. Also, is there any arguments taken in by the functions? E.g. execute(), addDueSoonTask

  3. For the cross that indicates a deletion, remember to place it a further ahead (not directly on the end of the lifeline bar)

  4. Consider color coding the different sections (e.g. model, logic, ui) for better readability

Comment on lines 286 to 288
Activity diagram for step 4:

image::TaskSummaryWithRefreshCommandActivityDiagram.png[width = "400", length = "600"]

Choose a reason for hiding this comment

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

The conditions that come after "find any task with expired status" should be more explicit, maybe you can consider [has expired status], [else].

Also, remember to join the node back after your conditions are executed.


Activity diagram:

image::CalendarActivityDiagram.png[width = "400", length = "600"]

Choose a reason for hiding this comment

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

I believe the command box for (day < Month.length) is not compliant with the activity diagram we were taught? Also, the if else condition should have square bracket.

Copy link

@duongphammmm duongphammmm left a comment

Choose a reason for hiding this comment

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

Overall good ideas, but maybe your implementation could be done with a bit more consideration and more detailed documentation!

@@ -79,15 +81,38 @@ image::UiClassDiagram.png[]

*API* : link:{repoURL}/src/main/java/seedu/address/ui/Ui.java[`Ui.java`]

The UI consists of a `MainWindow` that is made up of parts e.g.`CommandBox`, `ResultDisplay`, `PersonListPanel`, `StatusBarFooter` etc. All these, including the `MainWindow`, inherit from the abstract `UiPart` class.
The UI consists of a `MainWindow` that is made up of parts e.g.`BrandingLabel`, `TaskListPanel`,

Choose a reason for hiding this comment

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

Consider changing the name of the classes at least to make them consistent with your application (from Person to Task)

As a more OOP model, we can store a `Tag` list in `Address Book`, which `Person` can reference. This would allow `Address Book` to only require one `Tag` object per unique `Tag`, instead of each `Person` needing their own `Tag` object. An example of how such a model may look like is given below. +
+
image:BetterModelClassDiagram.png[]

[[Design-Storage]]

Choose a reason for hiding this comment

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

Similarly, change class names here as well to make it consistent!


Step 1. The user launches the application for the first time. The `VersionedAddressBook` will be initialized with the initial address book state, and the `currentStatePointer` pointing to that single address book state.
Command: add

Choose a reason for hiding this comment

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

Should add the syntax of the command instead of just the command word itself, e.g.: add n/ NAME t/ [TAG]...

image::UndoRedoState4.png[]
Step 1. The user launches the application with some alive and archived task records in the `storage`.
Statistics will perform on the records retrieved, and the result will be visualized as different charts organized in a `tab panel`.
After the user clicks `Statistics -> Task Summary`, the `tab panel` will display.

Choose a reason for hiding this comment

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

This might not be complying to the project requirement of CLI? is there a CLI command for this?


The task summary feature allows user to enjoy visualized and real-time statistics of their tasks' information.

This feature is implemented using different charts as `pie chart`, `area chart`, `bar chart` and `line chart`.

Choose a reason for hiding this comment

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

I'm not sure how these charts are implemented, are they new classes or just some attributes? maybe for OOP, implement them as new classes and reflect that in model?

** Pros: Will use less memory (e.g. for `delete`, just save the person being deleted).
** Cons: We must ensure that the implementation of each individual command are correct.
==== Implementation
The calendar feature, as the name suggests, displays a calendar fo users.

Choose a reason for hiding this comment

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

typo here at "fo" 😢

Comment on lines 315 to 318
. Displays the name of all tasks on that day in the calendar grid.
. Access calendars in previous or next month using the buttons on top.
. Displays more information about tasks for the day on the due soon panel after clicking on a grid. (Not done)
. Fast forward to a specific year/month using CLI. (Not done)

Choose a reason for hiding this comment

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

same concern here about CLI

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.