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

[CS2113-T12-4] WellNUS++ #25

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

Conversation

wenxin-c
Copy link

@wenxin-c wenxin-c commented Mar 2, 2023

WellNUS++ is a Command Line Interface(CLI) app for NUS Computing students to keep track and improve their physical and mental wellness in various aspects. If you can type fast, WellNUS++ can update their wellness progress faster than traditional Graphical User Interface(GUI) apps.

Copy link

@ngkaiwen123 ngkaiwen123 left a comment

Choose a reason for hiding this comment

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

Description of design and implementation is generally well done! Good work! Some diagrams may need to be improve for better readability.

### Reflection Component

![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)
![Reflection Component Class Diagram](diagrams/ReflectionClassDiagram.png)

Choose a reason for hiding this comment

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

image
The dotted arrows appear to be a little confusing. Perhaps it would be possible to redraw them?

Comment on lines 218 to 243
`LikeCommand` class: <br>

- Command format: `like <index of question(1~5)>`
- Users can add an introspective question that is generated in the previous set into their favorite list. Since there
will only
be 5 questions per set, the indexes are restricted to integer 1~5.
- `addFavQuestion()` method in `QuestionList` class is used to add and store the data.
- Users can only successfully add a question to favorite list if they have gotten a set of questions previously.
- Every time a question is added into the favorite list, the indexes of this particular question will be stored in data
file straightaway. It prevents data loss due to unforeseen computer shutdown.

`FavoriteCommand` class: <br>

- Command format: `fav`
- Users can get the questions in their favorite list.
- `getFavQuestions()` method in `QuestionList` class matches the indexes to corresponding questions and return this set
of questions
back to `FavCommand` for output. As such, `QuestionList` is a dependency for `FavoriteCommand` as well.

`HomeCommand` class: <br>

- Command format: `home`
- This command allows users to return back to the main WellNUS++ interface.
- Similar to `GetCommand`, `validateCommand()` method will also be called to validate the command.
- It will then call the class-level method `ReflectionManager.setIsExit()` to terminate the while loop
in `Reflectionmanager`.

Choose a reason for hiding this comment

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

Perhaps it will also be great to include how these classes handle errors? Or if there are any cases (or zero cases) of errors they may need to handle?


![CommandParser implementation](diagrams/CommandParserSequence.png)

Choose a reason for hiding this comment

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

image
A reference frame for the CommandParser accessing its own methods inside the loop may help to better describe what is happening in the sequence diagram?


### AtomicHabit Component

![AtomicHabit Component](diagrams/AtomicHabit.png)

Choose a reason for hiding this comment

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

image
A redraw of the arrows to make them seem neater may be better for the reader?

#### Overview
The overall execution lifecycle of the WellNus application involves 4 main components, as shown in the diagram below.

![Application Lifecycle](diagrams/WellnusSequence.png)

Choose a reason for hiding this comment

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

image
Text on dotted lines should be the variable returned rather than a description?

Copy link

@ngyida ngyida 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 overall! The link to github is a nice touch :)


![Application Lifecycle](diagrams/WellnusSequence.png)

The application begins with a call to `WellNus.start()`, which initialises an instance of `MainManager` and calls the
Copy link

Choose a reason for hiding this comment

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

Could this be done without since it is a description of the sequence diagram?

Comment on lines 132 to 133
![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)
![Reflection Component Class Diagram](diagrams/ReflectionClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

This diagram is too crowded which makes it hard to read. Could you perhaps abstract some of the details out?

are not the focus of this section since they are outside of `reflection` package.<br>
<br>

#### Feature Package (`ReflectionManager`, `ReflectionQuestion`, `QuestionList`, `TextUi`, `RandomNumberGenerator` classes)
Copy link

Choose a reason for hiding this comment

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

Could consider the use of UML diagrams to represent class to improve clarity


#### Integration with WellNUS++

![Integration](diagrams/CommandParserClass.png)
Copy link

Choose a reason for hiding this comment

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

This class diagram may not follow the standard UML notation

### Reflection Component

![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)
![Reflection Component Class Diagram](diagrams/ReflectionClassDiagram.png)

Choose a reason for hiding this comment

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

The class diagram here could possibly be abstracted out and separated out into different features since it is not mandatory to include everything. Users may find it hard to read as the text size is a little small


### AtomicHabit Component

![AtomicHabit Component](diagrams/AtomicHabit.png)

Choose a reason for hiding this comment

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

To simplify the diagram, maybe not every command has to be shown, which would definitely reduce the amount of arrows

* **Argument**: A word that is a parameter to a `Main Command` and is prefixed by ` --`. `e.g. --id, --name`
* **Payload**: An (optional) arbitrary sequence of characters immediately following a main command or argument.
The payload will terminate when the user clicks `enter` or separates the payload with another argument
with the `--` delimiter.

## Instructions for manual testing

Choose a reason for hiding this comment

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

Maybe this section could be removed from the developer guide? This is because your user guide already covers the commands, creating duplicate content.


For example,
`
$ foo bar --arg1 payload1 payload1--1 --arg2 payload2 --arg3

Choose a reason for hiding this comment

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

Maybe this could be included in a code block for easier visualization

* Instead of using electronics with fancy GUI, this CLI app gives computing students an opportunity to minimise digital
interaction which will be beneficial for their wellness.
* The app is gamified to make it more attractive for students to use. Levels and micro-goals incentivise our
users to keep using the app’s features, allowing them to focus on their work and achieve wellness.

## User Stories

Choose a reason for hiding this comment

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

User stories language format could be changed to fit the "I want to ..." sentence. For e.g. I want to use keyboard instead of mouse vs I want to I can use keyboard instead of mouse

### Reflection Component

![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)
![Reflection Component Class Diagram](diagrams/ReflectionClassDiagram.png)

Choose a reason for hiding this comment

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

diagram is messy and hard to visualise. might want to consider removing some of the less important parts and only showing the important features of the diagram

| v2.0 | Reflective student | I can get the previous questions I viewed | I can re-view these questions |
| v2.0 | Easily distracted computing student | I want to start a timer to keep track of time spent on work | I can do timed-practice |
| v2.0 | Easily distracted computing student | I want to check the time | I can keep track of my pace |
| v2.0 | A regular WellNUS++ user | I wish to have my information stored in the app | I can re-view my past data |

Choose a reason for hiding this comment

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

can add priority levels / level of importance to this

Copy link

@Stella1585 Stella1585 left a comment

Choose a reason for hiding this comment

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

(sorry i forgot to submit my review)
developer guide is very informative!


### Reflection Component

![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)

Choose a reason for hiding this comment

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

I feel that this might be a little repetitive as each command is executed in a similar way. It also ends up being very long and looks complicated. Maybe you can separate the commands.

### Reflection Component

![Reflection Component Class Diagram](diagrams/ReflectionSequenceDiagram.png)
![Reflection Component Class Diagram](diagrams/ReflectionClassDiagram.png)

Choose a reason for hiding this comment

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

The association lines are all overlapping, and it is difficult to tell them apart, especially at the top where the label and lines overlap. once again i suggest you can separate into perhaps feature and command diagrams.

The application begins with a call to `WellNus.start()`, which initialises an instance of `MainManager` and calls the
`MainManager.runEventDriver()` method.

`MainManager.runEventDriver()` will then take control of user input and provide a basic interface that parses commands

Choose a reason for hiding this comment

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

There are mentions of keywords and user inputs that are being tackled but i do not see a string being passed through or a text ui class, maybe u can show this in your digram too?


![CommandParser implementation](diagrams/CommandParserSequence.png)

Choose a reason for hiding this comment

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

Well made sequence diagram, helps me to understand how a command is parsed.

YongbinWang and others added 25 commits April 8, 2023 19:38
Replace 'Implementation Rationale' with
  'Design Considerations' to standardise
  these section headers.
wenxin-c and others added 30 commits April 10, 2023 20:25
…rror

Remove extra space printed for MainManger when invalid command is given
…Parser

Fix developer guide typos command parser
Fix single-letter variable name
Update DeveloperGuide Table of Contents
Fix list indices in DeveloperGuide
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.

10 participants