-
Notifications
You must be signed in to change notification settings - Fork 5
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
Quiz feature #62
Quiz feature #62
Conversation
* 'master' of https://github.com/joshruien/tp: Add testing for ListCommand update for CheckStyleTest Add testcases for Find command Add ability to hide answers change find format update TestCases files left out UserGuide update for find function javadoc update Find function find tag function find question and find answer Change URL of HelpCommand window Change ClearCommand success message
* branch-quiz: Implement quiz select random question
Includes Quiz command, Answer command and Exitquiz command
@@ -73,4 +73,8 @@ shadowJar { | |||
archiveName = 'medmoriser.jar' | |||
} | |||
|
|||
run { | |||
enableAssertions = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, totally missed out the requirement for tp
* Creates an AnswerCommand | ||
* @param answer The user's input answer. | ||
*/ | ||
public AnswerCommand(String answer){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may want to change the parameter to userAnswer for consistency
*/ | ||
public class QuizCommand extends Command { | ||
|
||
public static boolean isQuiz = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the isQuiz state should be placed in the command. Do you think it would be possible to put it under a new class that controls the system's state? @jonfoocy what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very sure about this either, I just added a getter and setter for isQuiz and made it a private static boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point actually I did the same thing for my implementation of the ListCommand. Maybe we can extract it and put it in the UiManager component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor changes to some of the parameter names, and I'm not sure if the state of isQuiz should be extracted and placed in another class rather than the QuizCommand, what do you think about this? Great job though!
Add getter and setter for isQuiz.
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
============================================
- Coverage 72.12% 68.57% -3.56%
- Complexity 440 442 +2
============================================
Files 74 79 +5
Lines 1324 1394 +70
Branches 138 148 +10
============================================
+ Hits 955 956 +1
- Misses 335 396 +61
- Partials 34 42 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
First iteration of the quiz feature for v1.3