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-T10-2] TermiNUS #28

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

Conversation

kelvneo
Copy link

@kelvneo kelvneo commented Sep 30, 2021

TermiNUS is a CLI (command line interface) program for NUS Students who wish to organize their NUS academic materials through a CLI. The product aims to aid student in organizing their academic schedule and enhancing their learning experiences.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@00c0fa2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #28   +/-   ##
=========================================
  Coverage          ?   88.38%           
  Complexity        ?      535           
=========================================
  Files             ?       65           
  Lines             ?     1532           
  Branches          ?      145           
=========================================
  Hits              ?     1354           
  Misses            ?       87           
  Partials          ?       91           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00c0fa2...e332e01. Read the comment docs.

Copy link

@pos0414 pos0414 left a comment

Choose a reason for hiding this comment

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

Just some minor issues with sequence diagrams of your DG. Other than that, looks good!


Initialization of all the classes required for Terminus class to run

![](attachments/MainLogic.png)
Copy link

Choose a reason for hiding this comment

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

Is this format correct? Should there be a : in front of the class name?
image

- accessing the arraylist of contents

### 3.7 Active Recall Component
![Active Recall Class Diagram](attachments/ActiveRecallClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Since there is already a direct association in the same direction as the dependence, can the dashed line be omitted?
image

#### 2.1.2 Getting the project files

Go to [link](https://github.com/AY2122S1-CS2113T-T10-2/tp) and retrieve the `TermiNUS project file`.
You can do so by **forking** the project and **cloning** a copy into your computer.

Choose a reason for hiding this comment

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

Would it be better to just mention "clone", since it is not compulsory that others fork the project to peek into it?


> 💡 IntelliJ IDEA have certain shortcut key to aid in auto-formatting of code.
> Once you are done with a piece of code, highlight the section you have just written and press the
> key `CTRL + SHIFT + L`.

Choose a reason for hiding this comment

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

For keys, would you consider using <kbd> HTML tags so that they stand out better? An example: Ctrl + Shift + L



### 3.3 Parser Component
![](attachments/ParserClassDiagram.png)

Choose a reason for hiding this comment

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

Would it be better to disable the icons, as per Prof's words?



### 3.4 Command Component
![](attachments/CommandClassDiagram.png)

Choose a reason for hiding this comment

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

For UML diagrams, it is also a solution to use online endpoints to generate the image from your source on the fly. This reduces the need to have binary files and keeps everything in one place.

Copy link

@ngnigel99 ngnigel99 left a comment

Choose a reason for hiding this comment

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

(DG review during tutorial)
Overall, great effort in producing the diagrams and elaboration. However, I think there are some minor changes you guys can make to improve the diagrams!


### 3.1 Architecture

![](attachments/MainInit.png)

Choose a reason for hiding this comment

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

Is this segment missing elaboration? I think it would be better to give a brief summary or explanation of the diagram


Initialization of all the classes required for Terminus class to run

![](attachments/MainLogic.png)

Choose a reason for hiding this comment

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

Is the ref portion of this diagram necessary? I'm not sure how it interacts with the whole system
image

Choose a reason for hiding this comment

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

Can the note part here be replaced with something more representative of a sequence diagram?
image


### 3.5 Module Component

![](attachments/Module.png)

Choose a reason for hiding this comment

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

I think this could just be a *
image

### 3.8 Storage Component


![](attachments/StorageComponent.png)

Choose a reason for hiding this comment

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

Similar to the previous recommendation, I think this could just be a *
image

Copy link

@KZQ1999 KZQ1999 left a comment

Choose a reason for hiding this comment

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

Overall pretty good job with the diagrams , just make it smaller so its easier for users and try to follow the UML notations in lectures


#### 4.2.1 Current Implementation

![Active Recall Sequence Diagram](attachments/ActiveRecallSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Could the sequence diagram be broken down into different parts using reference frame, as it might be too big to follow

- Provides functionality to list all commands for the help `Command`


### 3.4 Command Component
Copy link

Choose a reason for hiding this comment

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

Perhaps the notation for visibility should be + and - as taught instead of green circles and boxes


To view the high-level diagram, head to [3.8 Storage](#38-storage-component).

#### 4.5.1 Initialize Storage Implementation
Copy link

Choose a reason for hiding this comment

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

good use of reference frame to simplify the diagram !

- Printing string arrays to the output through `printSection()`.


### 3.3 Parser Component
Copy link

Choose a reason for hiding this comment

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

Perhaps you should not use the circular icon beside the class instances and use : instead


#### 4.1.1 Current Implementation
The following sequence diagram shows how the timetable feature works:
![](attachments/TimetableSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

I believe theres something wrong with the nusmodule activation bar returning a module without being called first
the leftward dashed arrow.

Copy link

@leeyikai leeyikai left a comment

Choose a reason for hiding this comment

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

Looks great overall, just minor changes


## 3. Design

### 3.1 Architecture

Choose a reason for hiding this comment

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

Would be better to have a brief explanation of what is going on! A brief intro of the different parts will be great.

Comment on lines +146 to +150
The Ui Component consists of the `Ui` class which handles all input and output operations within
TermiNUS application. To reduce coupling, we have used `Ui` on only the main runner `Terminus`, and
the Active Recall `GameEnvironment`. If future features require the extended use of `Ui`, they may
call `getInstance()` from `Ui` to get the same singleton class as both `GameEnvironment` and
`Terminus`.

Choose a reason for hiding this comment

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

Clear and good explanation! Very easy to follow

Comment on lines 219 to 229
The Active Recall Component consists of the `GameEnvironment` as the centre of the design.
The `GameEnvironment` consists of a `QuestionGenerator` which will only exist if there is a
`GameEnvironment`, and a `Ui` instance to handle user input and printing of information. The
decision to re-use the `Ui` is to allow easier upgrades to the `Ui` if there is a need in the
future.
The `QuestionGenerator` takes in a list of `Question` and a maximum question count to randomly
generate questions based on `Random`. If `Random` is not provided, a new `Random` with a random seed
will be created to generate the `Question` order.
The `DifficultyModifier` is a utility class used to calculate and tweak the weights of `Question`
after the user has provided feedback on the difficulty of the question. It uses a
[logistic curve](https://en.wikipedia.org/wiki/Logistic_function) to calculate the change in weight.

Choose a reason for hiding this comment

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

Might be good to have bullet points at the bottom to guide the reader through as this is quite a lot of info

Comment on lines 349 to 368
The reason for using `NavigableMap` to generate questions was because it provides a method called
`.higherEntry(key)`, which guarantees a `Question` is returned provided the value never exceeds
the total weight of the question pool (which should never happen as the random number generator can
only generate between `0` and `total`).

To prevent the user from viewing the answers before they are ready, we have decided to require the
user to press Enter to display the answer as a method of confirmation, as it is the most effective
way to ensure the answer does not get revealed unless the user intents to view it.

The rationale behind using the logistic curve is to ensure that as harder questions appear more
often and easier questions appear less. We also want to ensure once the user finds a hard question
easy, it should quickly move down a difficulty and vice versa. The application of the logistic curve
also prevents the values from increasing too rapidly, and dominating the question pools. It also
prevents the case where easy questions disappear entirely from the question pool due to low weights.
We will continue to seek user feedback and tweak the curve parameters if needed if there are any
issues.

![](attachments/desmos-graph.png)
The parameters of the logistic curve can be viewed here:
[https://www.desmos.com/calculator/qefovvnuhx](https://www.desmos.com/calculator/qefovvnuhx).

Choose a reason for hiding this comment

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

Clear and great explanation!

kelvneo and others added 30 commits November 7, 2021 21:40
Change Logger error behaviour and separate StorageManager from Logger
Update naming in sequence diagram
Update UG with rationale for CS2101
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