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-W12-1] QuizHub #8

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

Conversation

yeo-menghan
Copy link

@yeo-menghan yeo-menghan commented Oct 4, 2023

QuizHUB is a local desktop app designed to help NUS students easily record examinable questions and generate quizzes from their very own question bank to test their understanding via a Command Line Interface (CLI). Easily launchable on the go, QuizHUB is a versatile tool that aims to streamline and optimize the revision experience for NUS students from all fields of study.

Copy link

@000verflow 000verflow left a comment

Choose a reason for hiding this comment

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

Overall well done diagrams, but with minor issues/redundant parts. Good job!

4. `[difficulty]` is the difficulty of the question for sorting later (i.e. Hard)

*Condensed Class Diagram - Does not contain all attributes & methods
![](./UML/AddShortCommand.jpg)

Choose a reason for hiding this comment

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

Should there be a link between Question and QuestionList?

Choose a reason for hiding this comment

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

image


#### Implementation of Edit Command

![commandEditSequence.png](UML%2FCommands%2FcommandEditSequence.png)

Choose a reason for hiding this comment

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

Should the return line be dotted?
image

Choose a reason for hiding this comment

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

For the CommandEdit Instance


#### Implementation of Edit Command

![commandEditSequence.png](UML%2FCommands%2FcommandEditSequence.png)

Choose a reason for hiding this comment

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

Could it be possible to omit the class names at the bottom, since it is repeated?
image


#### Class Structure of Start Command

![commandStartSequence.png](UML%2FCommands%2FcommandStartSequence.png)

Choose a reason for hiding this comment

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

Could it be possible to omit the class names as the bottom since it is repeated?
image

Copy link

@JoanneJo JoanneJo left a comment

Choose a reason for hiding this comment

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

Very detailed with many nice diagrams! Nice work!

`Commands` refer to a package of individual commands with complex and specific
logic, which is later executed in Quizhub and displayed by `UI`.

`Storage` is the class through which questions can be stored on the hard drive and `Utility Classes`
Copy link

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 place the 'Utility Classes' in a new line?


This starts a loop in which
user input is continuously read in from CLI for the user command to be extracted and executed.
In each iteration of the loop, `QuizHub` makes a call to `Ui.getUserInput()` and returns the
Copy link

Choose a reason for hiding this comment

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

The diagram says QuizHub calls "readCommand()" but the documentation says "getUserInput()"?

In each iteration of the loop, `QuizHub` makes a call to `Ui.getUserInput()` and returns the
entire user input as a String object. Following which, `QuizHub` makes a call to
`Parser.parseCommand()` to extract the user command from the String object and returns a
`Command` object. Finally, `QuizHub` makes a call to `Command.executeCommand()` and performs
Copy link

Choose a reason for hiding this comment

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

The diagram says QuizHub calls "execute()" but the documentation says "executeCommand()"?

All commands require a starting payload, but some commands do not require the subsequent arguments.

```
commandType [payload] [/argument1 [payload1] /argument2 [payload2] ... ]
Copy link

Choose a reason for hiding this comment

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

It seems like [ ] is used to indicate both required and optional inputs? Perhaps there is a better way to differentiate them if they are supposed to be different.

Comment on lines 232 to 233
`find /[description]` OR `find /[module]`
i.e. `find /water buffalo`, `find /CS2113`
Copy link

Choose a reason for hiding this comment

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

Perhaps the examples can be on a new line? It appears to be on the same line in the website. Maybe the format could be standardized for all the different commands.

Comment on lines 302 to 303
- `/all` will not require any input from `/[start details]`
3. The method `getAllQns()` from package `quizhub.questionlist.QuestionList` will be called to retrieve all questions from the storage list.
Copy link

Choose a reason for hiding this comment

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

Should '/all' be point 3 and 'getAllQns()' be a subpoint of '/all'? Perhaps 'categoriseListByModules' can also be written as 'categoriseListByModules()' just to standardise.

Comment on lines 308 to 309
3. /random will randomize the list of questions using `java.util.Collections.shuffle` and store it within a temporary array to prevent tempering with the original array in Storage
4. `/normal` will not requirwritee any further actions, using the previously generated list as specified by `/[quiz mode]` and `/[start details]`
Copy link

Choose a reason for hiding this comment

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

Perhaps /random can be written in a code block? I believe there is a typo "requirwritee".


#### Class Structure of Start Command

![commandStartSequence.png](UML%2FCommands%2FcommandStartSequence.png)
Copy link

Choose a reason for hiding this comment

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

Is the exit here same as the exit sequence diagram?


![](./UML/Storage.jpg)

:exclamation: This sequence diagram emphasizes the process of loading data into storage, and has therefore omitted details of more trivial and/or non-related methods as well as exception handling logic. To find out more about the details, please refer to the complete code and header comments.
Copy link

Choose a reason for hiding this comment

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

I think the icon is not showing.

Copy link

@ryan1604 ryan1604 left a comment

Choose a reason for hiding this comment

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

Overall very detailed DG, good job!

1. For the main structure of the program, we have adapted the project structure
from the Individual Project of Man Juncheng at [Link](https://github.com/spinoandraptos/ip/tree/master) </li>
2. For JUnit testing, we have adapted the testing codes from the AddressBook level-2
codes at [Link](https://github.com/se-edu/addressbook-level2)</li>
Copy link

@ryan1604 ryan1604 Nov 2, 2023

Choose a reason for hiding this comment

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

Is </li> supposed to show on the DG?

are the QuizHub, Ui, Parser and Command packages.

![](./UML/architecture.jpg)

Copy link

Choose a reason for hiding this comment

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

I like the addition of an architecture diagram so other developers can easily understand the overall structure of the product

Comment on lines +204 to +211
Lists all the questions in the current QuestionList.

When executed, the command will invoke the QuestionList.printQuestionList method.
This method first checks whether the list is empty and prints and error message if empty,
else it will invoke the QuestionList.printQuestion method on each Question object,
with the asList parameter set as true. The QuestionList.printQuestion method will then
print each question with a given index, and indicate the question's type and completion status
(obtained through Question.getQuestionType() and Question.questionIsDone() methods)
Copy link

Choose a reason for hiding this comment

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

I think this can be formatted better, eg. using QuestionList.printQuestionList


#### Class Structure of Start Command

![commandStartSequence.png](UML%2FCommands%2FcommandStartSequence.png)
Copy link

Choose a reason for hiding this comment

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

Should the other "display exit message" be dotted line instead?

image

#### Class Structure of Markdiff Command
![commandMarkDiffSequence.png](UML%2FCommands%2FcommandMarkDiffSequence.png)

![commandMarkDiffClass.png](UML%2FCommands%2FcommandMarkDiffClass.png)
Copy link

Choose a reason for hiding this comment

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

The arrows here are for inheritance, I think it should be something like "->" instead


#### Class structure of Command Exit

![commandExitSequence.png](UML%2FCommands%2FcommandExitSequence.png)
Copy link

Choose a reason for hiding this comment

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

Should the return arrow be a dotted line?

image

Copy link

@AlWo223 AlWo223 left a comment

Choose a reason for hiding this comment

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

Very detailed DG, make sure to follow the correct notations closely


## Application Lifecycle

![](./UML/lifecycle.jpg)
Copy link

Choose a reason for hiding this comment

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

Is the specific command instance still relevant after the execution? You might consider marking the end of its lifetime with a red X at the bottom of the activation bar.
Screenshot 2023-11-02 at 17 19 57


The following sequence diagram shows the implementation of `parseCommand`.

![](./UML/Parser.jpg)
Copy link

Choose a reason for hiding this comment

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

I suggest reducing the amount of commands included in the uml diagram as it will improve readability and still convey the same idea.


#### Implementation of Edit Command

![commandEditSequence.png](UML%2FCommands%2FcommandEditSequence.png)
Copy link

Choose a reason for hiding this comment

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

The return arrow for the commandEdit instance should be dotted to comply with the notation covered in the lecture. The same adjustment should me conducted for the lifelines of the different objects.


#### Implementation of Edit Command

![commandEditSequence.png](UML%2FCommands%2FcommandEditSequence.png)
Copy link

Choose a reason for hiding this comment

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

In order to closely follow the convention learned in the lecture, you should delete the "calls" and "returns" statements. It could be visualised by using full vs. dotted lines instead
image


#### Class Structure of Start Command

![commandStartSequence.png](UML%2FCommands%2FcommandStartSequence.png)
Copy link

Choose a reason for hiding this comment

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

Is your UI calling the parse start command in the implementation? This might either be a wrong representation in your code or it might be a good idea to separate the execution of logic function like parsing from the UI. Use the UI for displaying messages only for very good OOP.
image


#### Class Structure of Start Command

![commandStartSequence.png](UML%2FCommands%2FcommandStartSequence.png)
Copy link

Choose a reason for hiding this comment

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

The notation of the Ui, CommandStart, ... objects should start with a ":" and optionally an object name. In case of static methods you should add <> above the class name in the diagram

CommandExit -> Ui: Signals the program to exit
Ui -> Storage: Saves any unsaved data
Storage --> Ui: Data saved successfully
Ui -> Ui: Displays exit message
Copy link

Choose a reason for hiding this comment

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

You should add an additional activation bar when calling a method inside the same class (see screenshot below):
image

Copy link

@choonkit-nus choonkit-nus left a comment

Choose a reason for hiding this comment

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

The developer guide is quite extensive and the documentation is broken into neat sections that shares more the about implementation of specific components, nice!

The main execution of the QuizHub application will concern 4 components which
are the QuizHub, Ui, Parser and Command packages.

![](./UML/architecture.jpg)

Choose a reason for hiding this comment

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

The architecture diagram is quite clear and concise, nice! The explanation for what each component does is really useful too

Comment on lines 191 to 193
*Condensed Class Diagram - Does not contain all attributes & methods
![](./UML/AddShortCommand.jpg)

Choose a reason for hiding this comment

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

Perhaps add the multiplicities and relations between the different classes


The process of loading data from the storage file specified in the constructor takes places in a few steps. To illustrate the overall flow on loading data, refer to the sequence diagram below.

![](./UML/Storage.jpg)

Choose a reason for hiding this comment

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

Is it ok for the methods to create the nested activation bar in Storage like this?

This command provides users with the ability to randomize the sequence of questions, which can be useful for
creating randomized quizzes or study sessions.

![ShuffleToStorage-Shuffle_to_Storage_Flow.png](UML%2FCommands%2FShuffleToStorage-Shuffle_to_Storage_Flow.png)

Choose a reason for hiding this comment

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

Is it possible to use a UML notation and diagram to represent this instead?

yeo-menghan and others added 30 commits November 13, 2023 20:50
Edits to DG uml diagrams size
Edits to DG uml diagrams size for overall flows
Reformat size for Edit Command diagrams
Update UG and bug in parsing edit command
Fix bug in edit command to disallow forward slash
Fix bug in edit command again
Update CommandShuffle when array empty
Update broken link in userguide
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