[CS2113T-F12-3] E-Duke-8 #57
Conversation
docs/DeveloperGuide.md
Outdated
| 4. Deleting an existing note | ||
| 5. Listing out all notes in a topic | ||
|
|
||
|  |
There was a problem hiding this comment.
Should Note implement Displayable?
docs/DeveloperGuide.md
Outdated
|
|
||
| **Deleting a note:** | ||
|
|
||
| This task is performed by the `NoteList.add()` method. |
There was a problem hiding this comment.
Should this be NoteList.delete()? I did notice the presence of a delete() in NoteList
docs/DeveloperGuide.md
Outdated
|
|
||
| **Listing topics in TopicList:** | ||
|
|
||
|  |
There was a problem hiding this comment.
The :Ui.printFoundTopics() does not point back to any activation bar. Perhaps it is supposed to point to :TopicList or :TopicCommand?
Also, is :TopicCommand supposed to be deleted (i.e X at the end of activation bar)?
Lastly, should :Topiccommand have a dotted line pointing left at the end of the activation bar?
docs/DeveloperGuide.md
Outdated
| 4. Deleting an existing note | ||
| 5. Listing out all notes in a topic | ||
|
|
||
|  |
There was a problem hiding this comment.
Perhaps the class names should be be underlined in the class diagrams?
Consider showing the return type of the functions for the add and delete too
There was a problem hiding this comment.
Alright thanks for the note on return types! However, I believe only object names for object diagrams have to be underlined.
docs/DeveloperGuide.md
Outdated
|
|
||
| **Listing topics in TopicList:** | ||
|
|
||
|  |
There was a problem hiding this comment.
You might want to consider putting some explanation before the diagrams and maybe even in between to describe the functions and the difference between the 2 diagrams shown here or separate the diagrams completely. It's a little confusing as the title of this component is listing topics in topiclist. It seems like a single method is calling the 2 functions depicited in the diagram? I think you should add the caller at the start of the diagram to show exactly what is calling TopicCommand and clarify on the differentiation between the 2 commands shown.
| The `Option` object stores one option of a question while the `OptionList` object stores all 4 options of the same | ||
| question. The class diagram below illustrates the structure of both classes. | ||
|
|
||
|  |
There was a problem hiding this comment.
Since OptionList is holding Option, you might have missed out on some link or dependency between the 2.
There was a problem hiding this comment.
Sorry for the confusion, but in our implementation options implements displayable and OptionList is actually holding onto displayable now.
| When the user launches the app, the main program will initialize a `TopicsStorage` object and call the `load` method | ||
| which will return a `TopicList` object. The following sequence diagram shows how the load operation works: | ||
|
|
||
|  |
There was a problem hiding this comment.
This diagram is very complicated. You could consider having annotations within the diagram or even split the diagram up into digestible bits and explain the loops piece by piece. For example the parseToOptionObject could have been just abstracted as a function and then explained in another diagram afterwards for clarity.
There was a problem hiding this comment.
Thank you for the feedback, I have simplified the diagram down to show only Topic and Option construction
|
|
||
| #### 2.3.1. Design of Parser | ||
|
|
||
|  |
There was a problem hiding this comment.
I like the comprehensive diagram, unfortunately, on the github.io page, the diagram's words appear to be too small. Perhaps you can consider hiding the members of each class, instead, explaining each command individually?
|
|
||
| Below is the sequence diagram for how the Parser component of `Eduke8` works with commands to show output to the user. | ||
|
|
||
|  |
There was a problem hiding this comment.
- Should :MenuParser be deactivated right at "return new HelpCommand()"?
- Perhaps you can consider not splitting :Eduk8's activation bar, that is to say, a full activation bar from "ui.getInputFromUser()" to the end. Should there also be a return dotted line pointing from :Eduk8 at the end of the activation bar?
- Should there be a return dotted line from :HelpCommand to :Eduk8 after command.execute()?
- Should :Ui return be pointing to an activation bar?
There was a problem hiding this comment.
- Is there a missing "e" in Eduke8?
- Should the return new HelpCommand() dotted line start from the bottom of the :MenuParser activation bar instead?
| TopicList is an ArrayList of type Displayable, which is one of two interfaces implemented | ||
| in the code for EDuke8. As such, many of the commands that manipulate the TopicList make | ||
| use of the package java.util.ArrayList. The TopicList is used to store Topics. | ||
|
|
There was a problem hiding this comment.
I think its a bit redundant to mention that topicList make use of the arraylist package, because you have already mentioned at the start that it is an arrayList of type Displayable
| 5. Listing out all notes in a topic | ||
|
|
||
|  | ||
|
|
There was a problem hiding this comment.
Might be better to put the description and some info about the figure first and then reference to the figure, rather than just display the figure without any prior warnings or minimal descriptions on it
docs/DeveloperGuide.md
Outdated
| Step 3: The `Ui.printTopicList()` method then prints out the description of each topic in the | ||
| `TopicList`. | ||
|
|
||
| **Finding a topic in TopicList:** |
There was a problem hiding this comment.
This entire section may be a tad bit wordy, would be better if there was some way you could present this info and the section before this in a figure or table as well. The use of the methods as well as the condensed words has me reading a few times to try to understand and remember, whereas in a figure I could look at it immediately and understand
|
|
||
| #### 2.3.1. Design of Parser | ||
|
|
||
|  |
There was a problem hiding this comment.
This may be too small, possibly because it's so wide, maybe consider changing the orientation of the diagram a bit so that it would be bigger?
docs/DeveloperGuide.md
Outdated
| called `QuizParser`. Given below is an example usage scenario of how the command parsing feature works at each step, | ||
| when the user types in input to get help in order to see what commands are available to the user. | ||
|
|
||
| Step 1. The user launches the program for the first time. The `MenuParser()` will be initialised and awaiting the user’s |
There was a problem hiding this comment.
I think you guys should standardise your formatting, small things like whether you want to use step 1: or step 1. I realised the step and the numbering is not always consistent. However its a small issue, just a penny for your thoughts
| When the user launches the app, the main program will initialize a `TopicsStorage` object and call the `load` method | ||
| which will return a `TopicList` object. The following sequence diagram shows how the load operation works: | ||
|
|
||
|  |
There was a problem hiding this comment.
Wow thats alot of nesting, I understand that it is nested quite a few times because there are very few topics and questions? Maybe consider an alternative in the long term that the questions and topics may increase. At the same time, the diagram is comprehensive but it is overwhelming. Effort has to be put in just to understand in. Maybe try splitting it up abit and abstracting it to give greater clarity and readibility.
There was a problem hiding this comment.
Thank you for the feedback, I have simplified the diagram down to show only Topic and Option construction
|  | ||
|
|
||
| ### 2.2. Model Component | ||
|
|
There was a problem hiding this comment.
Could you consider writing a short preface here? Just to briefly explain what will be covered in this section.
| ### 2.2. Model Component | ||
|
|
||
| #### 2.2.1. Design of TopicList | ||
|
|
There was a problem hiding this comment.
Similar to above, could you give a short preface here? Perhaps to introduce what the list below is describing.
docs/DeveloperGuide.md
Outdated
| TopicList is an ArrayList of type Displayable, which is one of two interfaces implemented | ||
| in the code for EDuke8. As such, many of the commands that manipulate the TopicList make | ||
| use of the package java.util.ArrayList. The TopicList is used to store Topics. |
There was a problem hiding this comment.
Perhaps some of these variables and class types could be formatted as code snippets. This would ensure continuity with the other sections below, where this has been done.
| TopicList is an ArrayList of type Displayable, which is one of two interfaces implemented | |
| in the code for EDuke8. As such, many of the commands that manipulate the TopicList make | |
| use of the package java.util.ArrayList. The TopicList is used to store Topics. | |
| `TopicList` is an `ArrayList` of type `Displayable`, which is one of two interfaces implemented | |
| in the code for EDuke8. As such, many of the commands that manipulate the `TopicList` make | |
| use of the package `java.util.ArrayList`. The `TopicList` is used to store `Topics`. |
docs/DeveloperGuide.md
Outdated
| Step 2: The `NoteList.add()` method makes use of `ArrayList` API, specifically the `ArrayList.remove()` method, to | ||
| delete the `Note` object in `NoteList`. | ||
|
|
||
| **Listing out all notes in a topic** |
There was a problem hiding this comment.
This heading could end with a colon to ensure continuity with the other headings
| **Listing out all notes in a topic** | |
| **Listing out all notes in a topic:** |
|
|
||
| The class diagram given below explains the high-level design of the Quiz system in E-Duke-8. Given below it is a quick overview of each component. | ||
|
|
||
|  |
There was a problem hiding this comment.
Similar to previous comments, could you consider enlarging this diagram? The text is slightly hard to read.
There was a problem hiding this comment.
Also, for this section of the diagram, the directions of the arrow and the caption do not seem to match. Should the direction of the dotted arrow be swapped, or the caption direction and description altered?
There was a problem hiding this comment.
Thank you for the feedback! I have enlarged the diagram slightly and also changed the direction of the label arrow!
|
|
||
| Below is the sequence diagram for how the Parser component of `Eduke8` works with commands to show output to the user. | ||
|
|
||
|  |
There was a problem hiding this comment.
- Is there a missing "e" in Eduke8?
- Should the return new HelpCommand() dotted line start from the bottom of the :MenuParser activation bar instead?
docs/DeveloperGuide.md
Outdated
| - [3. Product scope](#3-product-scope) | ||
| - [3.1. Target user profile](#31-target-user-profile) | ||
| - [3.2. Value proposition](#32-value-proposition) |
There was a problem hiding this comment.
Could all the key words in these headings be capitalised to be the same as the other headings?
| When the user launches the app, the main program will initialize a `TopicsStorage` object and call the `load` method | ||
| which will return a `TopicList` object. The following sequence diagram shows how the load operation works: | ||
|
|
||
|  |
There was a problem hiding this comment.
Could you consider making the initial arrow for the load() method longer? It might help to make it more visible and clearer
There was a problem hiding this comment.
I have made it longer as suggested, thank you for the advice.
docs/DeveloperGuide.md
Outdated
| Step 1: The `parseCommand()` method instantiates a `NoteCommand` object which then calls the `NoteList.add()` method. | ||
| A new `Note` object is passed into its parameter. | ||
|
|
||
| Step 2: The `NoteList.add()` method makes use of `ArrayList` API, specifically the `ArrayList.add()` method, to add |
There was a problem hiding this comment.
Should API be spelt out and defined? Just because it's the first time it's being used in this DG.
docs/DeveloperGuide.md
Outdated
| Step 1: The `parseCommand()` method instantiates a `TopicsCommand` object which then calls the | ||
| `TopicList.showTopics()` method. | ||
| Step 2: The `TopicList.showTopics()` method then calls the method `Ui.printTopicList()`. The | ||
| current `TopicList` is passed into the called method. | ||
| Step 3: The `Ui.printTopicList()` method then prints out the description of each topic in the | ||
| `TopicList`. |
There was a problem hiding this comment.
Consider inserting some line spacing for these lines so that they are rendered properly.
docs/DeveloperGuide.md
Outdated
|
|
||
| Through the logic in the object of `UserStatsCalculator`, necessary information regarding the user’s statistics, such as the `totalPointsGained` integer value and `totalPointsAvailable` integer value, are calculated, and then passed to the `Ui` object to print them to the user. This same concept and procedure are applied to the display the other aggregate results. | ||
|
|
||
| A similar procedure is being employed by the `TopicalStatsCalculator` object to calculate the topic-level statistics for the user. The only difference between the objects of these two classes is that instead of iterating through all the topics available, the ` TopicalStatsCalculator` object only deals with a particular topic at any point of time. By iterating through the questions of the single topic, it calculates statistics for the topic and returns it back to the `Stats` object, which will then pass them to the `Ui` object to display them to the user. As such, in order to display the user’s statistics for each and every topic, a loop is done in the `Stats` object to repeatedly calculate the topic-level information for all of the topics and displaying them concurrently. |
There was a problem hiding this comment.
Some formatting issues/mistakes here which causes confusion to the reader. Explanation is generally good but for better readability and neatness I think you should use point form to explain each object so that it's easier to differentiate each point.
There was a problem hiding this comment.
Thank you for spotting this mistake! The changes have been made 👍
docs/DeveloperGuide.md
Outdated
|
|
||
| An object of `SingleTopicQuiz` class represents an instance of the quiz in E-Duke-8. Its `numberOfQuestions` attribute and `Topic` object correspond to the user's specified number of questions and topic for the quiz respectively. | ||
|
|
||
| The `startQuiz(:Ui)` method call from the `SingleTopicQuiz` object initializes an object of `QuizQuestionsManager` by passing into it `numberOfQuestions`, as well as an ArrayList of questions from the `Topic` object. The `QuizQuestionsManager` object will then randomly select `numberOfQuestions` questions from the topic the user has chosen, using its `setQuizQuestions(:int, :ArrayList<Displayable>)` method. |
There was a problem hiding this comment.
I feel that explanation on what parameters are taken in should be done more. The description of the parameters don't need to be too long, just briefly describe what kind of parameters are taken in. This might help to create a clearer picture in mind of how each method works
There was a problem hiding this comment.
Alright! Thank you for the feedback! Will look into this for v2.1!
|
|
||
|  | ||
|
|
||
| Results of the quiz attempts can be calculated using the information stored in a `Question` object, because of its methods, namely `wasShown()`, `wasHintShown()` and `wasAnsweredCorrectly()`, that indicate if it has been attempted before, whether hint was used when user attempted the question and if the question was answered correctly respectively. |
There was a problem hiding this comment.
Maybe have sub-titles so that each step is clearly defined.
There was a problem hiding this comment.
Alright! Will look into it for v2.1. Thanks!
docs/DeveloperGuide.md
Outdated
|
|
||
|  | ||
|
|
||
| As there is a high level of nesting in the JSON file, many methods are called in loops to parse each section and return them as objects which are then used to build the next object at a higher level. For example, a `Question` object requires an `OptionList` which is created with an `ArrayList<Option>` collection. Thus, `parseToOptionObject(optionAsJson)` must first be called to return the `Option` objects to be placed in the `ArrayList<Option>`. More properties can easily be added to the classes and the storage component in a similar way, by parsing in loops. |
There was a problem hiding this comment.
Due to the nested looping, it is a bit complicated so I feel more explanation should be done like why such a deeply nested method needs to be used.
|
|
||
| #### 2.3.1. Design of Parser | ||
|
|
||
|  |
There was a problem hiding this comment.
The diagram is very well made, but it is rather tough to view on the webpage. Perhaps you could split the MenuParser commands and QuizParser commands into two diagrams? The various commands could also be arranged around the abstract Command class as a semi circle or a square with no bottom so that the width of the picture can be greatly reduced, so maybe that will help with the issue.
docs/DeveloperGuide.md
Outdated
| The command parsing feature is our program’s way of reading the user’s input into the command line. It makes use of a | ||
| single method `parseCommand` that identifies what command the user is calling for and then calls the command. There are | ||
| two parsers in our program that implements a single `Parser` interface. One parser is for choosing menu options and is | ||
| named `MenuParser`. The other parser is used during quizzes, in order to answer questions or request for hints, and is | ||
| called `QuizParser`. Given below is an example usage scenario of how the command parsing feature works at each step, | ||
| when the user types in input to get help in order to see what commands are available to the user. |
There was a problem hiding this comment.
A well written explanation, but this text appears as a rather lengthy paragraph when viewed in the IO. Perhaps some formatting such as bulleting could be used to make it easier for the reader to understand the key points of the paragraph.
|
|
||
| Below is the sequence diagram for how the Parser component of `Eduke8` works with commands to show output to the user. | ||
|
|
||
|  |
docs/DeveloperGuide.md
Outdated
| Step 2. The user types in "help" into the command line interface and presses enter. This user input “help” is stored as | ||
| a string and is put into the `parseCommand()` method as a parameter, together with the list of topics. This | ||
| topic list is not relevant to the help command for now. | ||
|
|
||
| Step 3. The user input string is subjected to the `lang.string.trim()` and `lang.string.split()` functions of a string | ||
| in the Java libraries in order to remove redundant spaces around the input, and to discern the number of words | ||
| in the input. The `lang.string.split()` function uses a blank space string, “ “, as the delimiter to split the | ||
| string into its individual components. | ||
|
|
||
| Step 4. Each subsequent string separated by a space is stored in a string array named `commandArr`. The 0th index of the | ||
| `commandArr` array is the first word, the 1st index is the second word, and so on. In this case there is only | ||
| one word stored in the array, at the 0th index, which is “help”. | ||
|
|
||
| Step 5. The string at the 0th index is then used in a switch statement, where each case represents the different menu | ||
| options available. As such, the contents of the case with reference “help” is run, which is a return statement | ||
| containing a new `HelpCommand()`. This leads to the execution of the `help` command. |
There was a problem hiding this comment.
Well written explanation, but perhaps formatting techniques such as bolding could be used to put more emphasis on the the main point of each step? Sub-bullets could also be used for the further elaboration/operations that follow so that the main point of each step is more visible.
| The sequence diagram below shows how `QuizQuestionsManager` is implemented to achieve this for the scenario where the user indicates that he wants to attempt 5 questions from the topic on OOP, which translates to the `setQuizQuestions(5, questionsInTopic)` call: | ||
|
|
||
|
|
||
|  |
There was a problem hiding this comment.
The ending for :ArrayList could perhaps end with the return arrow.
There was a problem hiding this comment.
Thank you for spotting that! I have fixed it and will update the diagram soon!
| Given data for the topics and questions is loaded automatically from JSON (JavaScript Object Notation) files in the data folder. This is mainly facilitated through the `TopicsStorage` | ||
| class which handles accessing the file as well as converting from JSON into `Topic`, `Question` and `Option` objects. The class diagram below shows this relationship. | ||
|
|
||
|  |
There was a problem hiding this comment.
These lines are slightly confusing to look at, perhaps consider changing the placement of Topic, Question and Option(side by side so the arrows all go up and do not intersect each other).
| 2. **Import the project as a Gradle project**: Choose the option to import the project as a Gradle project when prompted. | ||
| 3. **Verify the setup**: Enter some commands to ensure E-Duke-8 functions as expected. Refer to our [User Guide](https://ay2021s1-cs2113t-f12-3.github.io/tp/UserGuide.html) for more information. | ||
|
|
||
| ## 2. Design & implementation |
There was a problem hiding this comment.
Consider adding a slight description for the overview of this topic.
|
|
||
| #### 2.2.3. Implementation of Notes | ||
|
|
||
| **Adding a new note:** |
There was a problem hiding this comment.
Could perhaps split Add, Delete and List into different sub topics to make it clearer.
Add bookmark delete UI, and edit bookmark list function
…nto update-StorageDG
…xtraWhiteSpaces Add parser logic to be able to ignore white spaces in input.
…ub.com/josephhhhhhhhh/tp into add-UpdatedJUnitTestsForParserBookmark
Bold words in PPP
Tweak PPP whitespace
…into add-amendmentsToUi
Update UG broken links
…into add-amendmentsToUi
…nto add-amendmentsToUi
Add amendments to ui
…ekuan/tp into add-DataResourceAsFallback
Add json to resources as a fallback
Change special chars to normal in UG
E-Duke-8 helps CS2113/T students learn and understand software engineering and OOP principles through a gamified platform and enhance their learning experience. It also consolidates key concepts for easy revision.