Luke's RPS Challenge #2116
Luke's RPS Challenge #2116
Conversation
| end | ||
|
|
||
| get '/play' do | ||
| @game = $game |
There was a problem hiding this comment.
I'm sure you had your reasons, but it looks to me like you could have avoided a global variable by using a session?
| end | ||
|
|
||
| post '/rps' do | ||
| @game = $game |
There was a problem hiding this comment.
Do you have to assign this twice? Already assigned on line 23.
| @turn = players[0] | ||
| @round = round | ||
| end | ||
|
|
There was a problem hiding this comment.
are player_1 and player_2 methods necessary? It feels like these can just be instance variables of just stay as items in the players array.
| attr_reader :players, :turn, :round | ||
|
|
||
| def initialize(player_1, player_2, round = Round.new) | ||
| @players = [player_1, player_2] |
There was a problem hiding this comment.
Would it have been cleaner to have two instance variables rather than an array?
| round.winner.increase_score | ||
| calculate_outcome_message | ||
| end | ||
| end |
| round.set_outcome('cuts') | ||
| end | ||
| end | ||
| end |
| self.class == Computer | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
This is fancy! So you can create an instance of computer and copy in all the methods and values from Player? Love it
There was a problem hiding this comment.
You demonstrated the learning objectives of the week - well done. I left a comment around your refactor of controller -> model, let me know what you think.
Testing of all possible combinations of player / comp moves to test the outcomes would also be a good addition in future projects like this.
| player2 = Computer.new | ||
| else | ||
| player2 = Player.new(params[:player_2_name]) | ||
| end |
There was a problem hiding this comment.
You mentioned finding it difficult to draw this logic into a Model - firstly, well done on achieving the functionality of the user stories!
One thing that may make this refactor simpler is thinking about what parts of your program actually need to know about the Player. It's the Game Model that does - in order to know Player 1's move, the Computer's move, or Player 2's move.
If your Game class was responsible for taking player name params as arguments when initialised, you could use a default argument for player 2 - defaulting to nil if no player 2 name is provided.
Additionally in your Game logic, the code that handles comparing the two player moves to calculate the winner, could then determine if it's going to be using player2's move or generating a computer move if player2 is nil.
What do you think?
| @game.turn.throw(params[:throw].to_sym) | ||
| @game.switch_turn | ||
| @game.act_for_computer | ||
| @game.calculate_outcome |
There was a problem hiding this comment.
This is a lot of method calls to keep in the route, when perhaps you could push the responsibility of calling each of these methods into the @GAMe instance, so you only need to call something like @game.generate_winner.
| private | ||
|
|
||
| def rps_logic | ||
| win_condition = { scissors: :paper, paper: :rock, rock: :scissors } |
There was a problem hiding this comment.
Concise handling of game winner logic.
Working 1 player vs computer version
Working 2 player vs player version