-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Luke's RPS Challenge #2116
base: main
Are you sure you want to change the base?
Luke's RPS Challenge #2116
Changes from all commits
41984de
ed17016
bf93d28
c4a190a
8360cc4
ea2f402
a929cdc
3aa0013
72a0870
1a45304
abdb26e
9e77045
6b9cf40
668d6fd
7221ce2
6927726
936480f
41b8c62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
require "sinatra" | ||
require "./lib/player" | ||
require "./lib/game" | ||
require "./lib/round" | ||
|
||
class RPS < Sinatra::Base | ||
get '/' do | ||
erb :index | ||
end | ||
|
||
post "/names" do | ||
player1 = Player.new(params[:player_1_name]) | ||
if params[:player_2_name].empty? | ||
player2 = Computer.new | ||
else | ||
player2 = Player.new(params[:player_2_name]) | ||
end | ||
$game = Game.new(player1, player2) | ||
redirect '/play' | ||
end | ||
|
||
get '/play' do | ||
@game = $game | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sure you had your reasons, but it looks to me like you could have avoided a global variable by using a session? |
||
erb :play | ||
end | ||
|
||
post '/rps' do | ||
@game = $game | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have to assign this twice? Already assigned on line 23. |
||
@game.new_round | ||
@game.turn.throw(params[:throw].to_sym) | ||
@game.switch_turn | ||
@game.act_for_computer | ||
@game.calculate_outcome | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
if @game.round.outcome.nil? | ||
redirect '/play' | ||
else | ||
redirect '/result' | ||
end | ||
end | ||
|
||
get '/result' do | ||
@game = $game | ||
erb :result | ||
end | ||
|
||
run! if app_file == $0 | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
require './app.rb' | ||
run RPS |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
class Game | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it have been cleaner to have two instance variables rather than an array? |
||
@turn = players[0] | ||
@round = round | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
def player_1 | ||
players.first | ||
end | ||
|
||
def player_2 | ||
players.last | ||
end | ||
|
||
def new_round | ||
if round.outcome_decided? | ||
@round = Round.new | ||
players.each { |player| player.reset_action } | ||
end | ||
end | ||
|
||
def switch_turn | ||
if turn == player_1 | ||
@turn = players[1] | ||
elsif turn == player_2 | ||
@turn = players[0] | ||
end | ||
end | ||
|
||
def act_for_computer | ||
if turn.computer? | ||
turn.random_throw | ||
switch_turn | ||
end | ||
end | ||
|
||
def calculate_outcome | ||
if player_1.thrown_action? && player_2.thrown_action? | ||
rps_logic | ||
end | ||
end | ||
|
||
private | ||
|
||
def rps_logic | ||
win_condition = { scissors: :paper, paper: :rock, rock: :scissors } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Concise handling of game winner logic. |
||
|
||
if player_1.action == player_2.action | ||
round.set_outcome("draws with") | ||
elsif win_condition[player_1.action] == player_2.action | ||
round.set_winner(player_1) | ||
round.set_looser(player_2) | ||
round.winner.increase_score | ||
calculate_outcome_message | ||
else | ||
round.set_winner(player_2) | ||
round.set_looser(player_1) | ||
round.winner.increase_score | ||
calculate_outcome_message | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DRY |
||
|
||
def calculate_outcome_message | ||
case round.winner.action | ||
when :rock | ||
round.set_outcome('smashes') | ||
when :paper | ||
round.set_outcome('wraps') | ||
when :scissors | ||
round.set_outcome('cuts') | ||
end | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like these messages |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
class Player | ||
attr_reader :name, :score, :action | ||
|
||
def initialize(name) | ||
@name = name | ||
@score = 0 | ||
end | ||
|
||
def throw(throw) | ||
@action = throw | ||
end | ||
|
||
def thrown_action? | ||
!!action | ||
end | ||
|
||
def reset_action | ||
@action = nil | ||
end | ||
|
||
def increase_score | ||
@score += 1 | ||
end | ||
|
||
def computer? | ||
self.class == Computer | ||
end | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fancy! So you can create an instance of computer and copy in all the methods and values from Player? Love it |
||
class Computer < Player | ||
def initialize(name = "Computer") | ||
@name = name | ||
@score = 0 | ||
end | ||
|
||
def random_throw | ||
@action = [:rock, :paper, :scissors].sample | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
class Round | ||
attr_reader :winner, :looser, :outcome | ||
|
||
def outcome_decided? | ||
!!outcome | ||
end | ||
|
||
def set_winner(winning_player) | ||
@winner = winning_player | ||
end | ||
|
||
def set_looser(loosing_player) | ||
@looser = loosing_player | ||
end | ||
|
||
def set_outcome(outcome_description) | ||
@outcome = outcome_description | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
feature 'users can input their choice of action' do | ||
before(:each) { players_sign_in("Luke", "Kirsty")} | ||
|
||
scenario 'user clicks on rock, oppenent selects scissors' do | ||
click_on 'rock' | ||
click_on 'scissors' | ||
expect(page).to have_content "Luke's rock smashes Kirsty's scissors" | ||
end | ||
|
||
scenario 'user clicks on paper, oppenent selects rock' do | ||
click_on 'paper' | ||
click_on 'rock' | ||
expect(page).to have_content "Luke's paper wraps Kirsty's rock" | ||
end | ||
|
||
scenario 'user click on scissors, oppenent selects paper' do | ||
click_on 'scissors' | ||
click_on 'paper' | ||
expect(page).to have_content "Luke's scissors cuts Kirsty's paper" | ||
end | ||
|
||
scenario 'user clicks on paper, opponent selects paper' do | ||
click_on 'paper' | ||
click_on 'paper' | ||
expect(page).to have_content "Luke's paper draws with Kirsty's paper" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
feature 'display scores' do | ||
scenario 'new game displays player1 and player 2 scores as 0' do | ||
players_sign_in("Luke", "Kirsty") | ||
expect(page).to have_content "Luke: 0" | ||
expect(page).to have_content "Kirsty: 0" | ||
end | ||
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 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?