-
Notifications
You must be signed in to change notification settings - Fork 22
Assignment 1 #17
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
base: Assignment1
Are you sure you want to change the base?
Assignment 1 #17
Conversation
added comment explaining update to questions array
Removed unnecessary comment to better adhere to Guidelines
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.
Assignment Details
Pull Request title includes "Assignment 1" in the title ✔
Partner has also made a commit ✔
Pull Request targets Assignment 1 branch ✔
Issue 1: Application no longer crashes ✔
Issue 2: Unit test properly passes ✔
Extra Credit
Come up with a new feature for the application
Issue 3: Add a new feature request implemented ✔
Issue 3: Feature request unit tested ✔
Pull Request title should really include the space between "Assignment" and "1"
public static bool ReplayQuiz() | ||
{ | ||
Console.WriteLine("Play again? (y/n)"); | ||
string input = Console.ReadLine(); | ||
return input != null && input.Trim().ToLower() == "y"; |
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.
Missing some kind of retry logic if the user does not enter a valid response. Consider if the users enters a key that is not 'y' or 'n', it should print some message saying "not valid response" and ask again.
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.
Colton,
Thank you for your feedback. I have implemented your suggestion in our latest update to our PR. A message indicating the input is invalid will now display until a proper response is input by the user.
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.
Pull Request Overview
This PR adds a replay feature to the Princess Bride Trivia game, allowing players to play multiple rounds without restarting the application. It also fixes a bug in percentage calculation and includes a missing line in the question loading logic.
- Added a do-while loop to enable quiz replay functionality
- Fixed percentage calculation to handle division correctly and provide formatted output
- Added missing assignment of questions to the array in LoadQuestions method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
PrincessBrideTrivia/Program.cs | Added ReplayQuiz method, wrapped main quiz logic in do-while loop, fixed percentage calculation, and added missing question assignment |
PrincessBrideTrivia.Tests/ProgramTests.cs | Added unit tests for the new ReplayQuiz method covering both 'y' and 'n' inputs |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Updated replay quiz function to handle incorrect inputs
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 was also thinking that randomizing the questions each time the user replays the quiz would be pretty cool too. Great work!
bool result = AskQuestion(questions[i]); | ||
if (result) | ||
Console.WriteLine("Play again? (y/n)"); | ||
string input = Console.ReadLine()?.Trim().ToLower(); |
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.
Instead of using Console.ReadLine
and treating the input as a string, the Console.ReadKey
method could be used. This avoids the need for trimming and converting the user input to lowercase. It's also pretty cool because you don't need to press enter after hitting "y" or "n"
A minimal snippet using ReadKey, which can be dropped into the inside of your while true loop (you should add the "else" case with the invalid input warning message, though):
ConsoleKey key = Console.ReadKey(intercept: true).Key;
if (key == ConsoleKey.Y)
{
Console.WriteLine("y");
return true;
}
if (key == ConsoleKey.N)
{
Console.WriteLine("n");
return 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.
Chander,
We did, in fact, have this in an earlier commit but ran into issues during unit testing.
#`while (true)
{
Console.WriteLine("Play again? (y/n)");
var key = Console.ReadKey(intercept: true).Key;
Console.WriteLine();
if (key == ConsoleKey.Y)
{
return true;
}
else if (key == ConsoleKey.N)
{
return false;
}
else
{
Console.WriteLine("Invalid input. Please enter 'y' to play again or 'n' to quit.");
}
}`
I do think it is the better way to check for input, so we will probably head back to the drawing board for the unit test so that we can put this back in the implementation. Thank you for your feedback.
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 just worked on this and managed to make it work with Console.ReadKey. It comes with some unit testing challenges, but I figured out a way to handle the test so we can use this approach
question.Answers[1] = answer2; | ||
question.Answers[2] = answer3; | ||
question.CorrectAnswerIndex = correctAnswerIndex; | ||
questions[i] = question; |
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.
On my pull request, a reviewer suggested cleaning up the object initialization for Question
I found that this simplified object initialization style worked really well & made this part of the code more readable.
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.
Chander,
I get a 404 error from your link, but I definitely see how many of the lines within this method can be combined and/or condensed. I'm going to do a bit of research on the type of initialization you mentioned and keep a better eye out for future assignments/projects for instances where I can use that.
SummarySummary
CoveragePrincessBrideTrivia - 64.4%
|
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.
Looks good!
if (key == ConsoleKey.Y) | ||
{ | ||
return true; | ||
} | ||
else if (key == ConsoleKey.N) | ||
{ | ||
numberCorrect++; | ||
return false; | ||
} | ||
else | ||
{ | ||
Console.WriteLine("Invalid input. Please enter 'y' to play again or 'n' to quit."); | ||
} |
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 like that y'all are reprompting the user for correct inputs. Nicely done!
Assignment Details
Extra Credit
I liked your test replay feature. Well done! |
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.
Assignment Details
- Pull Request title includes "Assignment 1" in the title ✔
- Partner has also made a commit ✔
- Pull Request targets Assignment 1 branch ✔
- Issue 1: Application no longer crashes ✔
- Issue 2: Unit test properly passes ✔
Extra Credit
- Come up with a new feature for the application
- Issue 3: Add a new feature request implemented ✔
- Issue 3: Feature request unit tested ✔
Question[] questions = LoadQuestions(filePath); | ||
do | ||
{ | ||
Question[] questions = LoadQuestions(filePath); |
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.
Probably don't need to reload the questions upon every quiz retake.
if (key == ConsoleKey.Y) | ||
{ | ||
return true; | ||
} | ||
else if (key == ConsoleKey.N) | ||
{ | ||
numberCorrect++; | ||
return false; | ||
} | ||
else | ||
{ | ||
Console.WriteLine("Invalid input. Please enter 'y' to play again or 'n' to quit."); | ||
} |
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.
Could use a switch statement here (I'm not sure if we've gone over that yet - just wanted to make you aware! :) ).
} | ||
|
||
|
||
public static bool ReplayQuiz(Func<ConsoleKey> readKeyFunc = null) |
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.
This isn't something I think we've gone over in class (could be wrong), so I won't deduct points. Just wanted you to be aware:
Adding functionality for testing purposes is bad practice as far as I'm aware. Though it might be beyond where we currently are to test it properly. The approach that is probably closest to where the class is, is redirecting stdin/out. This documentation might help.
@thigiang16 @katmilton Remember to respond to and resolve all peer review comments. Copilot's review too (though it didn't add any comments). |
I worked with @katmilton.
Extra credit: Added a new feature that lets players replay the quiz. After each round, they are asked if they want to play again, enter 'y' to play again and 'n' to quit