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

[CS2113T-M16-1] Task Scheduler #7

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

Conversation

tqiren
Copy link

@tqiren tqiren commented Mar 2, 2020

@nus-cs2113-AY1920S2 nus-cs2113-AY1920S2 deleted a comment from tqiren Mar 3, 2020
damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020
mvp menu with periods and style fixed
Copy link

@jiajuinphoon jiajuinphoon left a comment

Choose a reason for hiding this comment

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

Overall, this is a clear and detail developer guide, despite of the missing of the adding features which can be confusing while reading it. Clear sequence diagrams helped me to understand the features and the design consideration is easy to understand. However, the usage of several examples might confuse the developer / reader. Maybe can try to make the separation clearer to decrease the confusion ? Nonetheless a very good developer guide 👍 😄 ,

. Locate and select the `build.gradle` file, then click `OK`
. Click `Open as Project`
. Click `OK` to use the default settings provided

Choose a reason for hiding this comment

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

Good to include the setting up process to lead the user 😄

* If there are tasks in the existing task list, it will call the `clearList()` method from the `TaskList` class to clear the
existing taskList

Given below is an example usage of `clear done` command:

Choose a reason for hiding this comment

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

Some suggestion over here: because you tried to explain this feature with 2 example, you may consider to split it with a title named "Example 1: usage of clear all" and "Example 2: usage of clear done" and bold it to make a clear separation? Because it can be confusing as both have similar steps 😃

e.g. Typing `help` command and pressing kbd:[Enter] will list the commands present
.. Some example commands you can try to get familiar with *ATAS*:
* `help`: Lists the commands that *ATAS* supports.
* `assignment n/Assignment One m/CS2113T d/01/01/20 1600 c/Important Assignment`: Adds an assignment called *Assignment

Choose a reason for hiding this comment

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

Maybe can include a section for adding the event / assignment ? In the POV of a developer, if the developer accidentally ignored this sentence, he / she will feel lost because there are no specific instruction in the implementation section until EDIT feature.

*Step 4* +
The `execute()` method in the `EditCommand` class first gets an input from the user on the details of the edited task.

[TIP]

Choose a reason for hiding this comment

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

Maybe the add feature / the parameters can be mentioned earlier / elaborate the adding function?
Reason: to avoid potential confusion. Developer may be confused that add function is a sub function in Edit function


The following sequence diagram summarizes how repeat command operation works: +

image::RepeatCommand_UML.png[Repeat Command Sequence Diagram]

Choose a reason for hiding this comment

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

Clear sequence diagram on explaining this features ! 👍
Possible minor fix: The arrow (circle in red) is disjoint from the repeatCommand box.
Screenshot (9)_LI


The following sequence diagram summarizes how delete command operation works: +

image::delete.png[delete task]
Copy link

@jiajuinphoon jiajuinphoon Mar 30, 2020

Choose a reason for hiding this comment

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

Clear sequence diagram to show the features ! 👍
Possible fix: Maybe can describe the if else condition with more words ? Because some may get confused about the [0] in the diagram 😺


The following sequence diagram summarizes how the `ClearCommand` operation works: +

image::clear.png[clear command]
Copy link

@jiajuinphoon jiajuinphoon Mar 30, 2020

Choose a reason for hiding this comment

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

CLear sequence diagram to describe this feature 👍
Possible minor bug: The arrow seems to be placed at a wrong place ? it should be pointed to the top right corner.
Screenshot (10)_LI

=== Model Component
The Model component contains the `Task` and `TaskList` classes, which store the user's schedule.

image::TaskList Task class diagram.PNG[TaskList and Tasks]

Choose a reason for hiding this comment

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

Good on using composition and correct multiplicity in this diagram 👍


The following sequence diagram summarizes how the `SearchCommand` and `SearchdCommand` works:

image::search.png[Search operations]

Choose a reason for hiding this comment

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

Clear sequence diagram to show the features ! 👍

In particular, the diagram below shows the explicit execution flow that `CalendarCommand` takes.

.Explicit execution flow of CalendarCommand
image::addMonthlyCalendar.png[]

Choose a reason for hiding this comment

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

Good description on this feature by using a sub sequence diagram to show the features ! 👍

Copy link

@GanapathySanathBalaji GanapathySanathBalaji left a comment

Choose a reason for hiding this comment

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

The overall DG was very good, well-formatted and easy to follow. Just a few minor issues with the UML diagrams. Could it be better if all the UML diagrams were standardized to follow a specific design? So that the report could look more cohesive.


The following sequence diagram summarizes how delete command operation works: +

image::delete.png[delete task]

Choose a reason for hiding this comment

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

  1. Maybe the condition label on the opt block can be improved to say that the list is empty instead of just mentioning a "0" for easier readability.

image


The following sequence diagram summarizes how delete command operation works: +

image::delete.png[delete task]

Choose a reason for hiding this comment

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

  1. Is this correct? The change of control from the Parser object to the Delete Comand isn't shown for the second transition.

Annotation 2020-03-31 122304


The following sequence diagram summarizes how the `SearchCommand` and `SearchdCommand` works:

image::search.png[Search operations]

Choose a reason for hiding this comment

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

  1. Is the object's name written in the correct format? Shouldn't it be ":SearchCommand" instead of "SearchCommand"?
    Annotation 2020-03-31 122733


The following sequence diagram summarizes how the `ClearCommand` operation works: +

image::clear.png[clear command]

Choose a reason for hiding this comment

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

  1. Is this correct? The control seems to change from TaskCommand to CommandResults even before the control reaches the function.

  2. If the CommandResult class is used for the 3 different alternative branches, shouldn't it be constructed/deleted outside the alt block? If not the class needs to initialized/deleted in all three branches separately.

  3. Shouldn't the self invocation arrows point to the top of the activation bar?

Annotation 2020-03-31 123402


The following sequence diagram summarises what happens when the `EditCommand` class is executed.

image::EditCommand_SequenceDiagram.png[]

Choose a reason for hiding this comment

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

Shouldn't the self invocation arrows point to the start of the activation bar, instead of a point below it?

Annotation 2020-03-31 124310

Copy link

@harithadiv harithadiv left a comment

Choose a reason for hiding this comment

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

Generally, a good DG with clear instructions and descriptions (:


## Design & Implementation
Content
- [Setting up](#1-setting-up)

Choose a reason for hiding this comment

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

Section 1 and section 3 both mentions setting up, could they be mentioned under the same section instead?

## 4. Implementation
This section will detail how some noteworthy features are implemented.
### 4.1. Delete Task Feature
Current Implementation:

Choose a reason for hiding this comment

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

Should there be a header for current implementation - ie. 4.1.1, as it divided into a subsection under search task feature, as section 4.2.1


The following sequence diagram summarizes how repeat command operation works:

![Repeat Command Sequence Diagram](images/RepeatCommand_UML.png)

Choose a reason for hiding this comment

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

In this UML diagram, the arrow representing return from method from CommandResult class to RepeatCommand class and arrow representing calling a method from RepeatCommand to CommandResult is not complete.

Expanding number of `Calendar` rows. This will require the need to increase the number of `Calendar` columns to preserve the integrity of a traditional calendar view. However, this also is infeasible as our goal is to keep the calendar compact such that it does not need to fill the screen.

## 4.7. Storage
### 4.7.1. Implementation

Choose a reason for hiding this comment

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

Could the header font size of subsections be kept consistent?


The `CalendarCommand` class extends the `Command` class with methods to implement the necessary pre-processing to display an overview of tasks in the given date. The following sequence diagram outlines an example execution of `CalendarCommand` when it is called and the interaction it has with the relevant components.

![Interaction of CalendarCommand and the various major components](images/calendar-diagram.png)

Choose a reason for hiding this comment

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

Method call back arrow missing for getTasksbyYearMonth()


In particular, the diagram below shows the explicit execution flow that `CalendarCommand` takes.

![Explicit execution flow of CalendarCommand](images/addMonthlyCalendar.png)

Choose a reason for hiding this comment

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

Perhaps remove some methods to make the diagram clearer instead?

A summary of all the features available in *ATAS* can be found in <<Commands Summary>>. +
Refer to <<Features>> for the detailed instruction of the various commands of *ATAS*.

== Design

Choose a reason for hiding this comment

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

Would having an Introduction section before the Design section help readers understand the purpose of this document? Having the purpose, scope and target audience of the application may help readers understand what this application is made for and who it is targeting.

The `Atas` component integrates all the aforementioned components to run the overall application logic. +
The sequence diagram below shows how various components, broken down into the various classes, interact when the user enters a `help` command +

image::atas help command sequence diagram v3.PNG[Component interactions for help command]

Choose a reason for hiding this comment

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

Good use of sequence diagram! Easy to read with the help of colours.

4. The `Ui` class is used to show the `CommandResult` message to the user. +
5. The `Storage` object is used to save the new state of the application.

== Setting Up

Choose a reason for hiding this comment

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

Maybe this section is not necessary? As this document is for developers and there is already another section in the beginning on how to set up the project on their computer.
It also confused me when I looked at the table of content and saw two sections with the same heading 'Setting Up' so it may be a good idea to either remove this section and direct the developers to the UG on helping users set up the application or change the name of the section.

... Run the command `java -jar atas.jar`. You will be greeted with the welcome screen of *ATAS* in a few seconds.

== Implementation
This section will detail how some noteworthy features are implemented.

Choose a reason for hiding this comment

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

Would adding a section on the Add Task Feature be considered noteworthy too? As a developer, I would want to know how a task is added into the program, especially since the other features cannot be done without adding any tasks first.


=== Delete Task Feature

Current Implementation: +

Choose a reason for hiding this comment

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

Good documentation of the implementation of the Delete Task feature, along with the sequence diagram to aid in the understanding. It goes through the step by step of what happens in the background along with the exceptions that may be thrown and what happens afterwards. 👍

* Creating 2 separate classes for `SearchCommand` and `SearchdCommand`
** Rationale: +
To create 2 separate commands so that users can filter their search query more easily.
** Alternatives Considered: +

Choose a reason for hiding this comment

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

Maybe having a consistent format would be better? Having a full stop after every sentence
e.g.
image
There should be a full stop at the end of the "Use a search..." and the Cons bullet point.
This would make the document look more consistent.


The following sequence diagram summarizes how the `ClearCommand` operation works: +

image::clear.png[clear command]

Choose a reason for hiding this comment

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

Should the arrow to of the self-invocation method be not pointing to the top right corner of the method?
image
Here is the example given in our textbook:
image
Do check your other sequence diagrams for this error as well! 😅


[[calendar]]
.Sample output of Calendar Command
image::calendar2.png[]

Choose a reason for hiding this comment

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

Good use of screenshots for visualization 👍

In particular, the diagram below shows the explicit execution flow that `CalendarCommand` takes.

.Explicit execution flow of CalendarCommand
image::addMonthlyCalendar.png[]

Choose a reason for hiding this comment

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

The use of a sub sequence diagram to zoom into the method is really useful for the developers. Good! 👍

@@ -0,0 +1,665 @@
= A.T.A.S (Amazing Task and Assignment System) Developer Guide
:site-section: UserGuide
:toc:

Choose a reason for hiding this comment

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

Would numbering the table of content help the reader with the flow of the documentation?

The `Atas` component integrates all the aforementioned components to run the overall application logic. +
The sequence diagram below shows how various components, broken down into the various classes, interact when the user enters a `help` command +

image::atas help command sequence diagram v3.PNG[Component interactions for help command]

Choose a reason for hiding this comment

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

image

I'm not sure exactly when will Storage be called by Atas... Maybe can clarify if Storage is being called after every command, or before the program is terminated?

*Step 4* +
The `execute()` method will do 2 things:

* If there are no tasks in the existing task list, it will initialize a new `CommandResult` class that prints out an error

Choose a reason for hiding this comment

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

it will initialize a new ...

Perhaps you mean 'instantiate'?

Comment on lines +153 to +154
`TaskList` class to delete the task, based on the index. At the end of the execution, the `DeleteCommand` class will
initialize a new `CommandResult` class that prints out the success message for task deletion.

Choose a reason for hiding this comment

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

DeleteCommand class will initialize a new ...

Perhaps you mean 'instantiate`?


The following sequence diagram summarizes how delete command operation works: +

image::delete.png[delete task]

Choose a reason for hiding this comment

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

image

  1. I'm not sure if opt refers to the case with no task, or the case with tasks.
  2. Would alt be a more accurate representation of the program flow, since only one of the two cases will be executed?

Comment on lines +160 to +167
==== Design Considerations
* Calling `remove()` method in `deleteTask()` command of `TaskList` method instead of calling `remove()` method within
the `execute()` method of the `DeleteCommand` class
** Pros: Easier implementation for other classes that requires the same use.
** Cons: Increased coupling amongst classes, which makes it harder for testing.
** Rationale: We decided to implement it in such a way because we feel that the effects of increased coupling in such a
case is minimal and testing for related classes and methods are not affected much. Furthermore, such implementation also
allows us to keep all the related commands to the list of tasks within a class which keeps our code cleaner.

Choose a reason for hiding this comment

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

Good to see design consideration for the readers to understand your decision!

Expanding number of `Calendar` rows. This will require the need to increase the number of `Calendar` columns to preserve the integrity of a traditional calendar view.
However, this also is infeasible as our goal is to keep the calendar compact such that it does not need to fill the screen.

=== Storage

Choose a reason for hiding this comment

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

Perhaps can consider adding a sequence diagram to explain the program flow?

Comment on lines +2 to +17
:site-section: UserGuide
:toc:
:toclevels: 4
:toc-title: Contents
:toc-placement: preamble
:sectnums:
:imagesDir: images
:table-caption: Table
:stylesDir: stylesheets
:xrefstyle: full
:experimental:
ifdef::env-github[]
:tip-caption: :bulb:
:note-caption: :information_source:
:warning-caption: :warning:
endif::[]

Choose a reason for hiding this comment

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

Perhaps can use numbering instead of bullet points in content page, for consistency with the actual headers in DG?

This section will give a high-level overview of how various components in *ATAS* function and interact with each other.

=== Architecture
image::overall architecture.PNG[overall architecture]

Choose a reason for hiding this comment

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

This diagram gives a very good understanding of your program!

=== Model Component
The Model component contains the `Task` and `TaskList` classes, which store the user's schedule.

image::TaskList Task class diagram.PNG[TaskList and Tasks]

Choose a reason for hiding this comment

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

This diagram gives a very good understanding of your program! Also, all the arrows look correct to me!


=== Storage Component

image::storage.PNG[Storage Class Diagram]

Choose a reason for hiding this comment

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

This diagram gives a very good understanding of your program!


## Design & Implementation
Copy link

Choose a reason for hiding this comment

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

The format of the page looks weird feels like there is some space wasted on both sides making it difficult to read. Maybe it would be good to change to a different theme?

- `exit`: Exits **ATAS**.

## 2. Design
This section will give a high-level overview of how various components in **ATAS** function and interact with each other.
Copy link

Choose a reason for hiding this comment

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

It is not very clear what your application is trying to achieve. Maybe can add a short sentence summarising what your app does?

### 2.4. Model Component
The Model component contains the `Task` and `TaskList` classes, which store the user’s schedule.

![TaskList and Tasks](images/TaskList Task class diagram.PNG)
Copy link

Choose a reason for hiding this comment

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

Your class diagram could maybe can to a solid lined box? Since the notes uses solid lines box

![TaskList and Tasks](images/TaskList Task class diagram.PNG)

### 2.5. Storage Component
![Storage Class Diagram](images/storage.PNG)
Copy link

Choose a reason for hiding this comment

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

Since you are abstracting out the attributes, maybe can remove the attributes compartment? Will make the diagram more concise.

The `Atas` component integrates all the aforementioned components to run the overall application logic.
The sequence diagram below shows how various components, broken down into the various classes, interact when the user enters a `help` command

![Component interactions for help command](images/atas help command sequence diagram v3.PNG)
Copy link

Choose a reason for hiding this comment

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

I think your UI activation bar should not have a break in between? Because it is waiting for the return result from "help".

**Step 2**
The user enters `search t\{TASK TYPE} n\{SEARCH QUERY}` into the command line. Method `parseCommand()` from the `Parser` class will be called to parse the command provided.

**Step 3**
Copy link

Choose a reason for hiding this comment

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

Maybe can break down to more steps? This step is very complicated and hard to digest.

**Step 2**
The user enters `searchd t\{TASK TYPE} n\{SEARCH QUERY} d\{DATE}` into the command line. Method `parseCommand()` from the `Parser` class will be called to parse the command provided.

**Step 3**
Copy link

Choose a reason for hiding this comment

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

Similar to the comment above, maybe can make it more concise to make this step easier to understand?


The following sequence diagram summarizes how the `SearchCommand` and `SearchdCommand` works:

![Search operations](images/search.png)
Copy link

Choose a reason for hiding this comment

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

Maybe can add some lines to show how the CommandResult class is initialised?


In particular, the diagram below shows the explicit execution flow that `CalendarCommand` takes.

![Explicit execution flow of CalendarCommand](images/addMonthlyCalendar.png)
Copy link

Choose a reason for hiding this comment

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

Maybe could split up the diagram to a few parts for easier understanding?


#### 4.6.2. Design Considerations

- Duplicating `Task` objects instead of keeping the `RepeatEvent` as a single entity like how it is stored in the `TaskList`.
Copy link

Choose a reason for hiding this comment

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

I am not sure of this design works, but maybe you can use a class level attribute to differentiate between repeat events and task objects

lwxymere and others added 30 commits April 10, 2020 17:37
Test image size for github pages print to pdf
* master:
  Test image size for github pages print to pdf
  Fix Javadoc
  Minor additions to DG/UG
  Minor change to PPP. Add test for EditCommand in Parser
  Update PPP
  Update DeveloperGuide.md
  Update DeveloperGuide.md
  Update DeveloperGuide.md
  Update DeveloperGuide.md
  Fix bug when manually editing save file
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.