[CS2113T-T09-2] Ravi by wangqinNick · Pull Request #27 · nus-cs2113-AY2021S1/tp

wangqinNick · 2020-10-01T01:11:39Z

The Ravi application is a simple yet powerful task management system that is dedicated to providing NUS freshman a more efficient way to organise their modules and tasks.

Nazryl

Overall the UG looks clean and neat. Love how some features are being shown. Minor problem is that the photos should indicate what kind of diagram and an overall description of the diagram will help reading go easier on the developers!

Nazryl · 2020-10-29T05:13:19Z

+# Developer Guide for ra.VI

-## Design & implementation
+## Table of Contents


Missing table of contents.

Nazryl · 2020-10-29T05:19:57Z

+viewed as a simple layer of abstraction on top of Command and CommandResult to facilitate the execution of user
+commands. Command is a dependency of Parser as Parser creates Command objects to return to the main loop.
+
+![UML class diagram for Main Class](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainClassDiagram.png?raw=true)


Could add a picture caption for the Main Class object at the bottom of the picture.

Nazryl · 2020-10-29T05:24:37Z

+viewed as a simple layer of abstraction on top of Command and CommandResult to facilitate the execution of user
+commands. Command is a dependency of Parser as Parser creates Command objects to return to the main loop.
+
+![UML class diagram for Main Class](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainClassDiagram.png?raw=true)


If adding a private variable for Duke and TextUI, consider adding for the rest of the classes or just leave it out.

Nazryl · 2020-10-29T05:32:56Z

+however, are dependencies of Command as there are commands for using/manipulating each one of them. InputOutputManager
+and Command are then dependencies of the main class Duke.
+
+![UML class diagram for Data Family Classes](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/DataClassDiagram.png?raw=true)


I believe the initialization should be as follows
-undoStack: Stack
-moduleCode: String

Nazryl · 2020-10-29T05:42:12Z

+
+The main sequence diagram can be broken into three parts representing each of these phases.
+
+![Sequence diagram 1 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram1.png?raw=true)


Unused classes should be removed as it is uninformative.

Nazryl · 2020-10-29T05:44:52Z

+TimeTableManager may need user input to be initialised, and so it is placed in a validation loop.
+A welcome message is shown at the end of the initialisation phase.
+
+![Sequence diagram 2 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram2.png?raw=true)


I like how there are comments on the sequence diagram to explain what's going on.

The activation bar on both ends should be closed and not left protruding out of the picture.

EthanWong2212

Looks good overall

EthanWong2212 · 2020-10-30T06:31:45Z

+* DeleteTask - Deletes a task from the task list through `TaskManager.delete()`
+* DeleteModule - Deletes a module from the module list through `ModuleManager.delete()`
+
+![Sequence diagram for AddCommand](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/AddCommandSequenceDiagram.png?raw=true)


Should term "construct" above constructor arrows be omitted for creation of new objects as it is implied by UML diagram notation?

EthanWong2212 · 2020-10-30T06:35:45Z

+* `CapCommand.gradeConvert` - Takes the grade of a module and assigns it a value according to the NUS grading schematic
+* `CapCommand.calculateCap` - Uses a formula to calculate the user's current cap, with the user's total Module Credit taken and current CAP.
+
+![Sequence diagram for Cap Feature in Command class](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/CapCommandSequenceDiagram.png?raw=true)


Should there be an indication that methods in :ModuleManager are static?

EthanWong2212 · 2020-10-30T06:37:03Z

+* DeleteTask - Deletes a task from the task list through `TaskManager.delete()`
+* DeleteModule - Deletes a module from the module list through `ModuleManager.delete()`
+
+![Sequence diagram for AddCommand](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/AddCommandSequenceDiagram.png?raw=true)


Is the end of lifeline supposed to be directly at the end of the activation bar?

JohnNub

Overall the developer guide is very detailed and contains good information about the overall configuration of the program. The use of abstraction to handle parsing errors is one of the more unique implementations I've come across.

However, I did not see the design considerations for the features listed anywhere in the document. Perhaps that is something worth considering as an addition.

JohnNub · 2020-10-30T06:10:48Z

+# Developer Guide for ra.VI

-## Design & implementation
+## Table of Contents


Should the table of contents be filled out?

JohnNub · 2020-10-30T06:12:01Z

+## Table of Contents

-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+## Setting up


Should the individual section headings be numbered for clarity?

JohnNub · 2020-10-30T06:16:03Z

+## Setting up
+The following section describes how to set up ra.VI on your own computer.

+### Software Prerequisites


The glow effect on the chosen webpage style might make it hard to tell level 2 and level 3 headings apart. Numbering the sub-sections should help with this greatly.

JohnNub · 2020-10-30T06:29:32Z

+![Sequence diagram 1 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram1.png?raw=true)
+ra.VI's `start()` method initialises all the classes used by the main class, like TextUi, InputOutputManager, and more.
+TimeTableManager may need user input to be initialised, and so it is placed in a validation loop.
+A welcome message is shown at the end of the initialisation phase.
+
+![Sequence diagram 2 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram2.png?raw=true)
+The main loop is contained in the 'runCommandLoopUntilExitCommand()' method.
+The main loop follows the following steps:
+1. Get the user command as a string.
+2. Parse the command using Parser. Parser will return a Command object.
+3. Execute the Command object with the `execute()` method. This will do the necessary work and return a CommandResult object.
+4. If any data was changed, StateManager will run `saveState()` to facilitate undo commands.
+5. Finally, use the CommandResult object to show the result of the command to the user using TextUi.
+
+Note that the Command and CommandResult objects are destroyed at the end of the method. Additionally, the Parser object
+is destroyed after each use.
+
+![Sequence diagram 3 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram3.png?raw=true)


These diagrams are extremely complex and there is a lot of information. While a lot of effort has been put into annotating the information, I feel like it could be easier to comprehend if it was split into simpler subsections.

For example, since you only want to talk about the start() command at the beginning, you could do a diagram on which functions are triggered by start(), then for 2nd one you can cover all functions run by runCommandLoopUntilExitCommand(). Focusing on just these functions alone (and not what invokes the functions) would help reduce the amount of stacking activation bars as well as give you a nice simple diagram for the 1st one with less redundant classes. If you still want to cover the entire execution of main() and run() you can add those in a separate diagram with no additional classes to cover the broad level scope of the program invocation and functions called.

JohnNub · 2020-10-30T06:34:10Z

+* DeleteTask - Deletes a task from the task list through `TaskManager.delete()`
+* DeleteModule - Deletes a module from the module list through `ModuleManager.delete()`
+
+![Sequence diagram for AddCommand](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/AddCommandSequenceDiagram.png?raw=true)


I like this diagram a lot more than the previous one as it doesn't show any unnecessary classes making it much easier to read.
However, I couldn't find any references to prepareAddCommand() anywhere in the document. Could this be better explained to the reader?

JohnNub · 2020-10-30T06:37:00Z

+2. The user inputs `add -m CS2101` into ra.VI, as the user wants to note down a module named ‘CS2101’ and add it to their module list.\
+The Ui receives the input as a string. Parser parses the string, and thereafter the AddCommandParser, before creating an AddModuleCommand. 


Parser parses the string, and thereafter the AddCommandParser

This phrasing is rather ambiguous, is it implying that the string is parsed twice? What is the result of the first parsing step? Why are there two parsing steps? According to your document it is mentioned that the Parser will create a command, but this section appears to suggest that the Parser just forwards the string to another parser class.

Could this be better explained to reduce ambiguity?

JohnNub · 2020-10-30T06:38:55Z

+Given below is an example usage scenario and how the cap feature behaves at each step.  
+1. The user launches the application for the first time.  
+2. The user inputs `add -t Read book` into ra.VI, adding the task to the task list in TaskManager. The user keys in multiple other tasks of the following:
+* `add -t Return book -by 2-10-2020 1400`
+* `add -t Meeting`  
+
+3. The user inputs `add -m CS2113T` into ra.VI, adding the module to the module map in ModuleManager. The user keys in multiple other modules of the following:
+* `add -m CS2101`
+* `add -m CG2271`
+
+4. The user inputs `list -t`. The CommandResult returns  
+```
+1. Read book [x]
+2. Return book [x], by 02:00PM, Friday, 02 Oct 20 
+3. Meeting [x]
+```
+Step 5. The user inputs `list -m`. The CommandResult returns  
+```
+1. CS2113T: Software Engineering & Object-Oriented Programming: No grade yet
+2. CG2271: Real-Time Operating Systems: No grade yet
+3. CS2101: Effective Communication for Computing Professionals: No grade yet
+```
+


Should the formatting appear like this?
Why is it labeled "Step 5" when the first 4 points do not use the prefix term "Step"?

JohnNub · 2020-10-30T06:40:37Z

+Given below is an example usage scenario and how the grade feature behaves at each step.
+
+1. The user launches the application. The user inputs `add -m CS2101` into ra.VI, as the user wants to note down 
+a module named ‘CS2101’ and add it to their module list. This input is received by the Ui ,which processes it into 


Should the comma be attached to the "which"?

JohnNub · 2020-10-30T06:42:06Z


 ## Non-Functional Requirements

 {Give non-functional requirements}


Should this line be here?

JohnNub · 2020-10-30T06:43:14Z

+Step 4. The TimeTableAddCommand is executed, returning a `CommandResult` containing a success message if the Lessons have 
+been successfully added. Otherwise, an exception message will be shown explaining the exception to the user.\
+Common reasons for failure include:


The earlier 3 points do not use the "Step" prefix, should this be amended as well?

* Add missing features to user story table

Update UG to add in expected output

Fix DG based on Prof's comments

breakspace

Quick Fix: IOManager doesn't load NUSMods if data dir doesn't exist

…anch_DG

Cut more PPP

Amend toc for the gui

DG and PPP update

Ppp

Cut more

* commit '0d1ed258ba9d0aea04a2f2472f1ee044684ec825': Cut more Cut Cut more P Amend toc for the gui Fix merge conflict in DG, update PPP Cut more PPP Cut more spacing Updates for DG and diagrams; moved more stuff to Executor Trying out attribute Updates to Data section of DG

Update PPP

New UG

Fixed typos and conflicts

wangqinNick changed the title ~~Ravi~~ [CS2113-W12-3] Ravi Oct 1, 2020

okkhoy changed the title ~~[CS2113-W12-3] Ravi~~ [CS2113T-W12-3] Ravi Oct 29, 2020

okkhoy changed the title ~~[CS2113T-W12-3] Ravi~~ [CS2113T-T09-2] Ravi Oct 29, 2020

Nazryl reviewed Oct 29, 2020

View reviewed changes

EthanWong2212 reviewed Oct 30, 2020

View reviewed changes

JohnNub reviewed Oct 30, 2020

View reviewed changes

tobiasceg and others added 24 commits November 5, 2020 18:15

minor UG edit

c9000df

Update DeveloperGuide.md

d963160

* Add missing features to user story table

Update DeveloperGuide.md

a598c18

Merge pull request #238 from tobiasceg/Branch-TobiasSplit

6eb0ac4

Update UG to add in expected output

Fix command class, timetable class diagrams

e9ee0ce

Fix and split AddCommandSequence diagram

1a98a9e

Merge branch 'master' into DG

87a6013

Add Sean's PPP

3862422

Merge pull request #244 from Aseanseen/DG

0554539

Fix DG based on Prof's comments

Change all duke into ravi

89b62df

Fix gradle

e97716f

Merge branch 'master' into PPP

cd1f49f

Added PPP

0e08d30

Minor edit to UG to add

745735b

breakspace

Fix: IOManager doesn't load NUSMods if data dir doesn't exist

f4b11c4

Merge pull request #250 from f0fz/QuickIOManagerFix

228a755

Quick Fix: IOManager doesn't load NUSMods if data dir doesn't exist

Merge branch 'master' into branch_DG

818cc8a

Merge branch 'branch_DG' of https://github.com/amalinasani/tp into br…

e0fc5ad

…anch_DG

Merge branch 'branch_tests' into branch_PPP

1439072

Update amalinasani.md

72be768

Update amalinasani.md

a1623ae

Update amalinasani.md

0b288cb

add @author

3a37c73

abit more

6318605

Aseanseen and others added 30 commits November 9, 2020 23:41

Amend toc for the gui

b5fba15

Merge pull request #302 from Aseanseen/PPP

6a669ea

Cut more PPP

Cut more P

7bc334b

Merge branch 'master' into UG

b83f062

Merge branch 'master' into PPP

b966141

Merge pull request #305 from Aseanseen/PPP

5d0e9dc

Cut more PPP

Merge branch 'master' into DGUpdate

c816547

Merge branch 'master' into UG

3742c66

Merge branch 'master' into DGUpdate

c47bc5b

Merge pull request #304 from Aseanseen/UG

35ad96c

Amend toc for the gui

Merge branch 'master' into DGUpdate

cba63a0

Merge pull request #303 from f0fz/DGUpdate

7666e90

DG and PPP update

Cut

da8cc87

Merge branch 'master' into PPP

234805b

Merge pull request #306 from Aseanseen/PPP

232e594

Ppp

Cut more

07d4e75

Merge branch 'master' into PPP

8c298a5

Merge pull request #307 from Aseanseen/PPP

0d1ed25

Cut more

Update PPP

d8cac04

Merge pull request #308 from wangqinNick/master

d8a1e69

Update PPP

Added new UG

95d37b8

Small fix for UG

c21a8cf

Merge branch 'master' into NewUG

34220db

Merge pull request #309 from f0fz/NewUG

9aff505

New UG

Fixed user guide name

38a0606

Merged config.yml

610cbb1

Merged UserGuide

9a5819f

Merge branch 'master' into NewUG

03a23ae

Merge pull request #310 from f0fz/NewUG

dd01343

Fixed typos and conflicts


		The main sequence diagram can be broken into three parts representing each of these phases.

		![Sequence diagram 1 for Main loop](https://github.com/AY2021S1-CS2113T-T09-2/tp/blob/master/docs/diagrams/MainSequenceDiagram1.png?raw=true)

		2. The user inputs `add -m CS2101` into ra.VI, as the user wants to note down a module named ‘CS2101’ and add it to their module list.\
		The Ui receives the input as a string. Parser parses the string, and thereafter the AddCommandParser, before creating an AddModuleCommand.


		## Non-Functional Requirements

		{Give non-functional requirements}

Uh oh!

Conversation

wangqinNick commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nazryl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EthanWong2212 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnNub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wangqinNick commented Oct 1, 2020 •

edited

Loading