[CS2113T-T09-1] Sherpass #4

jltha · 2022-03-03T01:19:27Z

No description provided.

SeanHoWB

Overall looks good, but more can be added to the project in terms of diagrams and elaboration of different components

SeanHoWB · 2022-03-31T02:09:56Z

docs/DeveloperGuide.md

+Below is a sequence diagram of what happens 
+as the user wishes to see the schedule (timetable) for 25th May 2022:
+
+![](images/showScheduleForADate.png)


This image hard to see, will be better if you can use ref

SeanHoWB · 2022-03-31T02:12:16Z

docs/DeveloperGuide.md

+
+Sequence diagram for `Timer` when user starts and stops a timer:
+
+![TimerClassSD](https://user-images.githubusercontent.com/69501969/160768104-fa7e06e3-1be8-4387-b75d-ae4e79bca5b7.png)


is update command here a method? if so need to do the proper self invoke syntax, arrow itself, activate, unarrow itself, deactivate. Same as start

what causes this? what happens here after the command is ran? Its abit unclear from the diagram

SeanHoWB · 2022-03-31T02:21:39Z

docs/DeveloperGuide.md

+
+### Study session
+
+Timetable component consists of `Timer`, `Stopwatch`, `Countdown`, `TimerParser`, `TimerLogic` and various commands.

 ## Design & implementation


i suggest splitting design and implementation, where design can include stuff like architecture overview,class diagrams of the 4 major compnents above

Yzkkk · 2022-03-31T02:22:09Z

docs/puml/showScheduleForADate.puml

+
+hide footbox
+
+participant User as User


You have changed a colour for sequence diagram under timetable implementation, which is good and its clear to the reader that it falls under a different category. However, for the sequence diagram under the storage implementation section, you are still using red, so this may be confuse the reader and defeats the purpose of having colours in diagrams.

Yzkkk · 2022-03-31T02:27:43Z

docs/DeveloperGuide.md

+#### Design considerations for Timer class
+- Current implementation: Create `Timer` from scratch, using the sleep function of threads to keep 
+track of time
+  - Pros: Same overhead of needing to track the time left of the timer
+  - Pros: No need to follow Java’s `Timer` class syntax, which can be confusing at times
+  - Pros: Implementation is simple and straight-forward
+  - Cons: Have to manage how we interrupt the thread after stopping the timer
+- Alternative: Using Java's `Timer` class
+  - Pros: The way of keeping track of the time has already been implemented
+  - Pros: Using a standard library usually makes the program less prone to various errors
+  - Cons: Still have to implement a way to keep track of time for our purposes of pausing a timer, since the library 
+  provided by Java has no way of pausing the timer, only stopping it.
+


Yzkkk · 2022-03-31T02:32:33Z

docs/puml/showScheduleForADate.puml

+hide footbox
+
+participant User as User
+participant Main as Main


The diagram is hard to read, I suggest using ref and extract the interaction between Timetable and TaskList.

Yzkkk · 2022-03-31T02:37:59Z

docs/DeveloperGuide.md

+  - [Study Session Implementation](#study-session-implementation)
+  - [Timetable Implementation](#timetable-implementation)
+  - [Storage Implementation](#storage-implementation)


I suggest including the sub-headings as well, eg Timer Implementation, Study session usage scenario, Design considerations for Timer class to be under Study Session Implementation.

angyongming

Good use of visuals and sufficient use of examples! Explanations are clear and concise and the overall flow is very coherent and readable. Nice job :)

angyongming · 2022-03-31T03:12:13Z

docs/DeveloperGuide.md

+Below is a sequence diagram of what happens 
+as the user wishes to see the schedule (timetable) for 25th May 2022:
+
+![](images/showScheduleForADate.png)


I agree with sean, diagram is rather complicated and I think it would be better to use a reference frame and split this diagram into multiple diagrams to see each one clearer.

Following up from my comment of the previous diagram, the Main class seems to be reading the userInput?

angyongming · 2022-03-31T03:15:32Z

docs/DeveloperGuide.md

+`Storage#load()` - Loads a saved JSON file and returns an ArrayList of task
+
+The path of the JSON file is provided as a parameter in the constructor of `Storage` hence 
+there is no need for any parameters in the `Storage#load()`. Since a save file will be created in the 


Not sure if there is a formatting error here or if it was intentional but why is there a # betwen Storage and its load() method? Is it supposed to be Storage.load() in this case?

angyongming · 2022-03-31T03:17:18Z

docs/DeveloperGuide.md

 ## Product scope
 ### Target user profile


Take note that there is a minor formatting error here:

angyongming · 2022-03-31T03:22:14Z

docs/DeveloperGuide.md

+
+Sequence diagram for `Timer` when user starts and stops a timer:
+
+![TimerClassSD](https://user-images.githubusercontent.com/69501969/160768104-fa7e06e3-1be8-4387-b75d-ae4e79bca5b7.png)


Since your project has a UI class, shouldn't the UI class be responsible for taking in the userInput instead of the StudyCommand class?

Just saw the rest of the diagrams, I noticed a little bit of inconsistency here whereby StudyCommand takes in userInput for this diagram, but for the other diagram it is Main, and for another diagram it is the UI class

angyongming · 2022-03-31T03:26:28Z

docs/DeveloperGuide.md


+![](images/StorageCorruptedSD.png)


Following up on my comment on the previous diagram, the reading of userInput here is done by Ui class, perhaps would be better to standardize this across the other diagrams!

angyongming · 2022-03-31T03:30:26Z

docs/DeveloperGuide.md

+### UI
+
+UI component consists the `Ui` class which manages interaction (receiving inputs and showing outputs) between the user 
+and the application.
+
+### Storage
+
+Storage component consists `Storage` and `StorageParser` classes.
+`Storage` class handles loading, writing and saving
+data to and from a JSON file, such that users' data will be saved automatically.
+`StorageParser` class handles the
+parsing of JSON from the saved data file.
+
+### Timetable
+
+For components with more complicated use-cases (`Task` and `Timer`), we separate an extra Logic class to achieve better
+modularity, such that each class addresses a separate concern.
+
+Timetable component consists of `Timetable`, `Task`, `TaskList`, `TaskParser`, `TaskLogic` and various commands.
+
+### Study session
+
+Timetable component consists of `Timer`, `Stopwatch`, `Countdown`, `TimerParser`, `TimerLogic` and various commands.


Very clear overview of the 4 main components of the programme. Good job!

okkhoy

In general, the code is quite neat!
I have highlighted some potential issues that may need fixing.
My suggestion is for each of you to take ownership and fix the files that you guys wrote. That way, each of you can get credit for good code 😄

okkhoy · 2022-04-07T01:03:03Z

src/main/java/seedu/sherpass/command/AddCommand.java

+    }
+
+    /**
+     * Accept parsed user input for by date, in proper format.


Accepts ...

same thing across the comments

okkhoy · 2022-04-07T01:04:43Z

src/main/java/seedu/sherpass/command/Command.java

+
+public abstract class Command {
+
+    public abstract void execute(TaskList tasks, Ui ui, Storage storage);


This method needs a header comment as it is an abstract method, meaning you are forcing someone else to provide an implementation, and they need to know what to implement

okkhoy · 2022-04-07T01:05:55Z

src/main/java/seedu/sherpass/command/ShowCommand.java

+        if (dayInput != null) {
+            ui.showToUser("Here is the schedule you wanted:");
+            Timetable.showScheduleByDay(dayInput, taskList, ui);
+            return;
+        }


good use of guard clause 👍

okkhoy · 2022-04-07T01:06:38Z

src/main/java/seedu/sherpass/command/ShowCommand.java

+        case "decemeber":
+        case "dec":


would lead to problems where the user doesn't use 3-letter/full name, e.g., decem

Would it be fine if we make the 3-letter form very clear in the user guide?

okkhoy · 2022-04-07T01:07:01Z

src/main/java/seedu/sherpass/command/StudyCommand.java

+     * Method is called when user chooses to enter Study mode. User is able to start, pause and stop a timer in Study
+     * mode. Only one timer can be running at a time. User can leave Study mode by typing "leave".


doesn't follow the standard for header comments

okkhoy · 2022-04-07T01:13:01Z

src/main/java/seedu/sherpass/task/TaskList.java

+     * Returns
+     * Printed tasks applies to non-recurring tasks.
+     *


violates convention

okkhoy · 2022-04-07T01:15:19Z

src/main/java/seedu/sherpass/timer/Timer.java

+import seedu.sherpass.util.Ui;
+
+
+public abstract class Timer extends Thread {


All abstract methods in this class need header comments

src/main/java/seedu/sherpass/timer/TimerLogic.java

src/main/java/seedu/sherpass/timetable/Timetable.java

okkhoy · 2022-04-07T01:18:42Z

src/test/java/seedu/sherpass/util/parser/ParserTest.java

+public class ParserTest {
+}


Jun Lim ped fixes

2 tests are omitted from final build as they incur an error when run with other tests, but are able to run individually

# Conflicts: # src/main/java/seedu/sherpass/task/TaskList.java

https://github.com/BradenTeo/tp into branch-ClearExpiredOrDone

# Conflicts: # docs/UserGuide.md

Isaac v2.1 documentation

Yuzhong documentation

Daryl Fix typo

Braden Update PPP

Yuzhong documentation

Daryl Update PPP

Daryl Fix URL in PPP again

1) Fix typo 2) Update release link 3) Add definition to terminal

Jun Lim UG updates

Braden Update Quick Start

Braden Add page breaks to UG

Braden Fix page break for UG

SeanHoWB reviewed Mar 31, 2022

View reviewed changes

Yzkkk reviewed Mar 31, 2022

View reviewed changes

angyongming reviewed Mar 31, 2022

View reviewed changes

okkhoy reviewed Apr 7, 2022

View reviewed changes

jltha mentioned this pull request Apr 7, 2022

Jun Lim ped fixes AY2122S2-CS2113T-T09-1/tp#150

Merged

thedarie and others added 25 commits April 8, 2022 17:43

Add period in comment to fix checkstyle error

59b7507

Update UML diagrams

560e44c

Add some test cases for manual testing

228382d

Fix bug when updating task

13f6aea

Merge branch 'master' of https://github.com/AY2122S2-CS2113T-T09-1/tp

b5c8def

Merge branch 'master' into isaac-BugFixes

9f12a08

Merge pull request #150 from jltha/branch-v2.1-Fixes

ba3c75a

Jun Lim ped fixes

Update task section in developer guide

e0d6ab4

add JUnit test for Clear Command

abc8d34

2 tests are omitted from final build as they incur an error when run with other tests, but are able to run individually

fix Checkstyle issue

71a96ee

Merge branch 'master' into daryl-ImproveStorage

ee41d31

# Conflicts: # src/main/java/seedu/sherpass/task/TaskList.java

Merge branch 'master' into branch-ClearExpiredOrDone

8c42528

Update screenshots to match new output format

3f20e33

fix JUnit issue when loading the data/test.json file

162679e

Merge branch 'master' of https://github.com/AY2122S2-CS2113T-T09-1/tp

4218caa

Merge branch 'branch-ClearExpiredOrDone' of

c9d5582

https://github.com/BradenTeo/tp into branch-ClearExpiredOrDone

Fix more Checkstyle issues

f3d8f85

Add newline to the end of some files

6f0fb8e

Merge branch 'master' into daryl-DG

e8190fd

# Conflicts: # docs/UserGuide.md

Update sequence diagram for storage

dd5429d

Update PPP

8feb2c7

Update checking of deadline

e0feabb

Add javadocs for the constructor and method

c4e1b64

Add javadoc for various Timetable methods

c3cf8d2

Add javadoc for one of the TimetableLogic method

00fa30b

laiisaac and others added 30 commits April 11, 2022 22:29

Add blank lines between images to improve formatting

d2075c5

Add page break into PPP

d5ced33

Merge pull request #176 from laiisaac/isaac-BugFixes

7dfd461

Isaac v2.1 documentation

Add page break to PPP

d17825b

Remove unnecessary element

e6582c0

Merge pull request #174 from yuzhongng/yuzhong-Documentation

a5dff91

Yuzhong documentation

Fix typo in hyperlink

a9145cf

Fix typo

5748952

Merge pull request #178 from thedarie/daryl-DG

1b468ea

Daryl Fix typo

Add author tag

6b1f12b

Update PPP

234dc32

Minor issue in DG

75e485d

Merge pull request #177 from BradenTeo/branch-Update-PPP

09de85e

Braden Update PPP

Merge pull request #179 from yuzhongng/yuzhong-Documentation

24b80ed

Yuzhong documentation

Update URL in PPP

3761d0f

Fix typo in Braden's PPP

99652d9

Merge pull request #180 from thedarie/daryl-DG

6eba379

Daryl Update PPP

Fix URL in PPP

d95c7a1

Merge pull request #181 from thedarie/daryl-DG

7104008

Daryl Fix URL in PPP again

Update UG

4a57759

1) Fix typo 2) Update release link 3) Add definition to terminal

Merge branch 'master' into branch-v2.1-docs

bf8aaa5

Update Quick Start

7b7cd4a

Remove sentence for greater clarity in UG

1d6e9a6

Merge pull request #182 from jltha/branch-v2.1-docs

912ea9e

Jun Lim UG updates

Merge branch 'master' into branch-UG-fix-for-CS2101

1cfabf4

Merge pull request #183 from BradenTeo/branch-UG-fix-for-CS2101

2423363

Braden Update Quick Start

Add page breaks to UG

48c5f70

Merge pull request #184 from BradenTeo/branch-UG-fix-for-CS2101

bb7cfcf

Braden Add page breaks to UG

Fix page break for UG

db1b908

Merge pull request #185 from BradenTeo/branch-UG-fix-for-CS2101

d6491e0

Braden Fix page break for UG

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CS2113T-T09-1] Sherpass #4

[CS2113T-T09-1] Sherpass #4

jltha commented Mar 3, 2022

SeanHoWB left a comment

SeanHoWB Mar 31, 2022

SeanHoWB Mar 31, 2022

SeanHoWB Mar 31, 2022

SeanHoWB Mar 31, 2022

Yzkkk Mar 31, 2022

Yzkkk Mar 31, 2022

Yzkkk Mar 31, 2022

Yzkkk Mar 31, 2022

angyongming left a comment

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

angyongming Mar 31, 2022

okkhoy left a comment

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022

yuzhongng Apr 8, 2022

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022

okkhoy Apr 7, 2022


		Sequence diagram for `Timer` when user starts and stops a timer:

		![TimerClassSD](https://user-images.githubusercontent.com/69501969/160768104-fa7e06e3-1be8-4387-b75d-ae4e79bca5b7.png)


		public abstract class Command {

		public abstract void execute(TaskList tasks, Ui ui, Storage storage);

		* Method is called when user chooses to enter Study mode. User is able to start, pause and stop a timer in Study
		* mode. Only one timer can be running at a time. User can leave Study mode by typing "leave".

		import seedu.sherpass.util.Ui;


		public abstract class Timer extends Thread {


		![](images/StorageCorruptedSD.png)

[CS2113T-T09-1] Sherpass #4

Are you sure you want to change the base?

[CS2113T-T09-1] Sherpass #4

Conversation

jltha commented Mar 3, 2022

SeanHoWB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angyongming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okkhoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment