-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Russells - Rock/Paper/Scissors Challenge #2110
base: main
Are you sure you want to change the base?
Conversation
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.
Nice work on this! You've achieved the user stories and learning objectives of the week. There's a good range of both feature and unit tests.
Fully comprehensive unit testing would cover every possible combination of the game rules - which sounds long, but would provide 100% test coverage.
There's an opportunity to do some refactoring to reduce duplication of code - see inline comments.
@robot_guess = nil | ||
@pvr_results = nil | ||
redirect '/pvr' | ||
end |
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.
You trigger a different route depending on what move the player chooses, and each of these routes runs the same five lines of code - there's an opportunity here to re-use existing code, keeping your implementation DRY.
An alternative approach would pass the player's chosen move as a param instead, then passing this into sessions or an instance of Player, for example.
@computer_guess == :spock && @guess == :scissors || | ||
@computer_guess == :spock && @guess == :rock | ||
'Robot wins' | ||
end |
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.
There are a few approaches to the this game logic, and although your logic is easy to read, an alternative approach could be more concise: https://github.com/makersacademy/rps-challenge/blob/main/docs/review.md#use-of-ifelsif-conditionals-for-business-logic
First Two User Stories Complete.
If I had more time I would:
Refactored in newest commit