Jimmy Lyons RPS challenge #2119
Conversation
There was a problem hiding this comment.
A good starting point, but I think if you wanted to move onto some more complex functionality it might be hard considering the current use of a single class model, and global variables. With some refactoring and splitting the Game class that would leave the code in much better condition, and make it easier to add multiplayer functionality etc.
I have some thoughts on the use of symbols vs strings, but really liked your use of output array in the spec to test multiple outcomes in one test - very neat
| end | ||
| end | ||
|
|
||
| describe '# return_winner' do |
There was a problem hiding this comment.
Add tests for all possible outcomes, as only three are tested here
|
|
||
| subject(:game) { Game.new('rock') } | ||
|
|
||
| it 'creates instances of game' do |
There was a problem hiding this comment.
describe Game tests that there is a Game class and that it can be initialised so don't think you need this test as well
| end | ||
|
|
||
| def has_expected_output | ||
| outputs = ['Computer Wins!', 'Player Wins!', 'Draw'] |
There was a problem hiding this comment.
Nice use of an array for outputs rather than expecting on multiple individual strings - I hadn't though to do that
| end | ||
|
|
||
| post '/names' do | ||
| $player_name = params[:name] |
There was a problem hiding this comment.
Global variables are apparently evil - player should be split out into separate class to be injected into Game
|
|
||
| def initialize(choice) | ||
| @player_action = choice | ||
| @pairs = { rock: 'paper', paper: 'scissors', scissors: 'rock' } |
There was a problem hiding this comment.
Think this is for the return_winner method, so why is it stored as an instance variable for the class?
| end | ||
|
|
||
| def random_choice | ||
| ['rock', 'paper', 'scissors'].sample |
There was a problem hiding this comment.
Could store this as a constant as the three options won't change. Also consider using symbols as they are immutable and are an identifier of an action rather than a typed user input.
| <h1>Pick an action <%=$player_name%>:</h1> | ||
|
|
||
| <form action='/choice' method='post'> | ||
| <input type="radio" id="rock" name="choice" value="Rock"> |
There was a problem hiding this comment.
Why set value to 'Rock' when you then use value.downcase in the app to convert to lowercase? Could refactor that out
| end | ||
|
|
||
| post '/choice' do | ||
| $player_choice = params[:choice].downcase |
There was a problem hiding this comment.
You're using a button input in the form so the choice shouldn't= need to use .downcase
| end | ||
|
|
||
| get '/result' do | ||
| game = Game.new($player_choice) |
There was a problem hiding this comment.
Consider initialising the Game when the choice is created so that you can store the choice within a player and within the Game and avoid the evil global variables
Completed the basic functionality (with no frills).