[CS2113-F15-1] Player2113#55
Conversation
Added more loggers
Update User Guide
| Ui -> Parser : handleAnswerInputs(answer: String) | ||
| activate Parser | ||
|
|
||
| ref over Parser : get correct answer |
There was a problem hiding this comment.
i think there's a missing sequence diagram here? maybe you can include the sequence diagram (esp for ref), such as the get correct answer?
|
|
||
| box Results #EB9999 | ||
| participant "topicResults:Results" as Results #FE2727 | ||
| participant "allResults:ResultsList" as ResultsList #FE2727 |
There was a problem hiding this comment.
i think you need to hide the class at the bottom of the diagram?
| deactivate ResultsList | ||
|
|
||
| Ui -->[ | ||
|
|
There was a problem hiding this comment.
you may need to deactivate Ui activation in the end after -->[ ?
| Ui -->[ | ||
|
|
||
| destroy Results | ||
| destroy Parser |
There was a problem hiding this comment.
maybe you should also destroy allResults?
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
class may need to add + and - to show public and private?
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
after calling the methods, i think you may need to add return arrow, even if void?
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
some method invocation got no name, maybe need to add the method name on it, instead of putting it on the return arrow?
|
|
||
| Similarly, the following sequence diagram shows how the | ||
| `AnswerTracker` stores all the user answer inputs: | ||
|  |
|
|
||
| ## Glossary | ||
|
|
||
| * *glossary item* - Definition |
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Shouldn't there be a + or - to indicate whether the method is private, public or protected?
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Shouldn't the colons be standardized?
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
Perhaps you can include the else condition in the second box?
|  | ||
| > **Note:** The lifeline for Parser and Results should end | ||
| > at the destroy marker (X) but due to a limitation of PlantUML, | ||
| > the lifeline reaches the end of the diagram. | ||
|
|
||
| Similarly, the following sequence diagram shows how the | ||
| `AnswerTracker` stores all the user answer inputs: | ||
|  |
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} |
| Step 1. The user launches the application for the first time, | ||
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following sequence diagram shows how the `Results` for |
There was a problem hiding this comment.
Hi, the method calling solid line arrow for "new Results()" seems to not be pointing to the start of the activation bar.
|
|
||
| Similarly, the following sequence diagram shows how the | ||
| `AnswerTracker` stores all the user answer inputs: | ||
|  |
There was a problem hiding this comment.
I think the photo is not loaded correctly on the DG site
|
|
||
| Step 3. The user now wants to view their results by executing | ||
| the `results` command. | ||
|  |
There was a problem hiding this comment.
Photo not loaded for this as well
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Class diagram for topicList should not have the C icon present, as I don't think it follows the standards stated.
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Missing of indication of private and public methods/ variables with "+/-" etc
ZMinghuiZ
left a comment
ZMinghuiZ
left a comment
There was a problem hiding this comment.
Generally good job! The notations of the diagrams seems to be compliant with the convention covered in the course. Maybe could add more diagram to enhance the developer guide understanding?
|
|
||
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  |
There was a problem hiding this comment.
I like the use of ref block to make the sequence diagram more concise and readable.
Good use of different color to differentiate Logic and Result👍
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  | ||
| > **Note:** The lifeline for Parser and Results should end |
There was a problem hiding this comment.
Perhaps add an object to indicate who invoked `printChosenTopic` method would make the diagram more clear.
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  | ||
| > **Note:** The lifeline for Parser and Results should end |
There was a problem hiding this comment.
Perhaps add the explanation of the ref block would make it more comprehensible?
| Step 1. The user launches the application for the first time, | ||
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: |
There was a problem hiding this comment.
Perhaps add a line separator here to make the class diagram occupy a dedicated space would allow the format to be more readable.
| The explain feature either prints the explanation to 1 question or all questions in 1 topic. | ||
|
|
||
| The explain feature is facilitated by `Parser#processExplanationCommand`, which is called by `Parser#parseCommand` | ||
|
|
There was a problem hiding this comment.
Perhaps add some diagram here would allow better understanding of developers?
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
The sequence diagram has many parts and may be slightly hard to understand initially, it would be good if you could break it down into parts when explaining, means showing just small sections of the sequence diagram
| Step 2a: `Parser#processExplainCommand` first checks the number of parameters in the user command | ||
| by calling `Parser#checkIfTwoParameters`. | ||
| The, further processing of parameters is done by calling `Parser#getTopicOrQuestionNum`. | ||
| This is facilitated by calling `QuestionsListByTopic#getQuestionSet` to get all questions in the specified topic. |
There was a problem hiding this comment.
maybe you could swap 2b and 2a since 2a is more related to step 3
|
|
||
| ### Libraries | ||
|
|
||
| 1. Display formatted tables in the CLI - [ASCII TABLES](https://bethecoder.com/applications/products/asciiTable.action) |
There was a problem hiding this comment.
good that you included your references :)
| |--------|----------|---------------|------------------| | ||
| |v1.0|new user|see usage instructions|refer to them when I forget how to use the application| | ||
| |v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list| | ||
| | Version | As a ... | I want to ... | So that I can ... | |
There was a problem hiding this comment.
maybe you could categorise the user stories by priorities and include their priority into the userstories table
|
|
||
| 2. Topic selection menu and testing mode progress bar - [ProgressBar](https://github.com/ctongfei/progressbar) | ||
|
|
||
| ### References |
There was a problem hiding this comment.
Maybe remove references if there is nothing to add here
| ### References | ||
|
|
||
| ## Instructions for manual testing | ||
|
|
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
Consider removing getters and setters as their functions are quite obvious
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Consider reformatting such that the text is above the diagram for clarity
|
|
||
| Similarly, the following sequence diagram shows how the | ||
| `AnswerTracker` stores all the user answer inputs: | ||
|  |
There was a problem hiding this comment.
I like that you have tried to differentiate between Logic and Answers using the different colors, but I think that it makes it harder to read the diagram as some content is between the colored boxes.
Maybe consider just coloring the class instance boxes and adding a legend on the side.
|
|
||
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  |
There was a problem hiding this comment.
Good use of ref block to help improve readability!
|
|
||
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  |
There was a problem hiding this comment.
Wouldn't it be better to use opt block instead of alt block as there is only one path?
| The solution feature is facilitated by `Parser#processSolutionCommand`, which is called by `Parser#parseCommand` | ||
|
|
||
| > **OVERVIEW:** | ||
| >  |
There was a problem hiding this comment.
Have you maybe forgot to add [else] int the alt block?
nichyjt
left a comment
nichyjt
left a comment
There was a problem hiding this comment.
Hi team,
Overall good job at making the DG!
- Please take note of the syntax issues with the diagrams and
- There are some sections of the DG that are incomplete. Please finish them.
- Most of the diagrams are OK in complexity, but consider scaling down some of the sequence diagrams.
Remember: guiding principle from the website...
Keep diagrams simple. The aim is to make diagrams comprehensible, not necessarily comprehensive.
Please do refer to AB3 DG for inspiration and tips on how to better structure and format your content.
| > **Note:** The lifeline for Parser and Results should end | ||
| > at the destroy marker (X) but due to a limitation of PlantUML, | ||
| > the lifeline reaches the end of the diagram. |
There was a problem hiding this comment.
It should be possible to remove the bubbles at the bottom with some tinkering of PlantUML if I am not wrong. Alternatively, you may also crop the image.
|
|
||
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  |
There was a problem hiding this comment.
Activation bar might not have been terminated correctly. Missing the border line at the bottom!
|
|
||
| Similarly, the following sequence diagram shows how the | ||
| `AnswerTracker` stores all the user answer inputs: | ||
|  |
There was a problem hiding this comment.
Similarly, the activation bar here was not terminated correctly
|
|
||
| The following sequence diagram shows how the `Results` for | ||
| one question set is added to the `ResultsList`: | ||
|  |
There was a problem hiding this comment.
See if you can get reference frames to be opaque.
- For readability
- To adhere to textbook
Recall the textbook's notation on reference frames. This is a repeated issue for all your sequence diagrams
|
|
||
| Step 3. The user now wants to view their results by executing | ||
| the `results` command. | ||
|  |
There was a problem hiding this comment.
A few issues:
- Unterminated activation bar for Parser
- Ref blocks not opaque
- The complexity is on the slightly higher side for this sequence diagram. You might want to consider refactoring
| and proceeds to start a game with their chosen topic. | ||
|
|
||
| The following shows the class diagram for `topicList`: | ||
|  |
There was a problem hiding this comment.
Most of your peers have identified the bigger issues of the diagram, like the class circle and the access modifiers.
There was a problem hiding this comment.
Consider adding in a table of contents, similar to the reference Address Book DG
|
|
||
| ### References | ||
|
|
||
| ## Instructions for manual testing |
There was a problem hiding this comment.
Please include details about manual testing here
| > **OVERVIEW:** | ||
| >  | ||
|
|
||
| > > **Note:** The lifeline for Parser and Results should end |
There was a problem hiding this comment.
Might be intentional, but the double quote makes the rendering seem buggy (space between 'overview' and the note.
Add hardcoded questions
Fix formatting for PPP
Update ppp
Fix formatting errors UG, DG
Update website
fix format in UG
Fix clear bug
Remove unnecessary code
# Conflicts: # src/main/java/seedu/duke/Parser.java
Merge branch 'master' of https://github.com/hongyijie06/tp
Fix ^D bug in sayHi()
14 participants
Player2113 is a CLI based software to help CS2113 student revise concepts encountered in the course.