TDDing a Card Game part 6

Last time we dealt with how Player notifies game that it's stuck. I also expressed some concern that I haven't extracted the game class. This time we'll start with one more test for player then we'll look at extracting the game class from SpitPlayerRules. Along the way we'll see the Single Responsibility Principle.

One Last Player Tweak For Now

In the game of Spit there are two piles on which the players spit. Adding the second pile is the last little tweak I want to make to Player before I extract a game. I'll write a test to force the Player to spit on a second pile.

class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @player = Player.new(self)
    #Game double's state
    @pile_one = SpitPile.new(7)
    @stock = []
    @spit_reserve = []
  end

  def test_player_can_play_on_either_pile
    @pile_two = SpitPile.new(5)
    @player.hand = [ 2, 3, 2, 3, 4 ]
    @player.play
    assert_equal [7] , @pile_one.to_a
    assert_equal [5, 4, 3, 2, 3, 2], @pile_two.to_a
  end

Notice that no card in @player.hand can be played on @pile_one. That will force the player to spit on @pile_two. We should run the test to make sure it fails... It does. Looking at the current Player code it's pretty clear that we need to change the bold line below

class Player
  def spit
    card_to_play = @hand.pop 
    unless @game.spit_on_one(card_to_play)
      @invalid_moves += 1
      @hand.unshift(card_to_play)
    end
    if @invalid_moves == 5
      @game.stuck
      @invalid_moves = 0
    end
  end
end

to this

unless @game.spit_on_one(card_to_play) or @game.spit_on_two(card_to_play)

Of course we're assuming that @game responds to #spit_on_two. Let's make it so. Remember that @game is still just an instance of SpitPlayerRules

class SpitPlayerRules < Test::Unit::TestCase
  # game methods
  def spit_on_two( card )
    @pile_two.play card
  end
end

Now we'll run the tests and move on...

D'oh. We have two failing tests: #test_player_draws_when_hand_is_short and #test_when_stuck_player_asks_for_new_spit. Can you spot why? Hmm, maybe the error message will help. It's the same for both failures.

NoMethodError: undefined method `play' for nil:NilClass
    SpitTest.rb:70:in `spit_on_two'
    ./spit.rb:48:in `spit'
    ./spit.rb:38:in `play'
    SpitTest.rb:48:in `test_player_draws_when_hand_is_short'

Ah ha! Player is trying to #spit_on_two, but @pile_two is nil in all but the test we just wrote. So I'll add it to the setup and initialize it with something that isn't playable.

class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @player = Player.new(self)
    #Game double's state
    @pile_one = SpitPile.new(7)
    @pile_two = SpitPile.new(0)
    @stock = []
    @spit_reserve = []
  end
end

We're on a green bar so let's refactor. I notice that the logic in Player#spit is rather crufty and obtuse. Let's see if you agree.

class Player
  def spit
    card_to_play = @hand.pop 
    unless @game.spit_on_one(card_to_play) or @game.spit_on_two(card_to_play)
      @invalid_moves += 1
      @hand.unshift(card_to_play)
    end
    if @invalid_moves == 5
      @game.stuck
      @invalid_moves = 0
    end
  end
end

It looks like in this method Player spits a card then assess the situation. The Single Responsibility Principle (SRP) says that a class should have one and only one reason to change. I like to extend this down to the method level as well. In this case I think we should split this method in two. First I'll extract each conditional into it's own method. The tests still pass. Player#spit isn't pulling it's own weight, so I'm going to inline it. Test are still passing. I'll reorder the statements in the while loop of Player#play and then I'll show the code. Okay, the tests are passing and here's the code.

class Player
  def play
    while(!@hand.empty?)
      assess_situation
      draw
      spit_card
    end
  end
  def spit_card 
    card_to_play = @hand.pop 
    unless @game.spit_on_one(card_to_play) or @game.spit_on_two(card_to_play)
      @invalid_moves += 1
      @hand.unshift(card_to_play)
    end
  end
  def assess_situation
    if @invalid_moves == 5
      @game.stuck
      @invalid_moves = 0
    end
  end
end

Smashing SpitPlayerRules with SRP

Speaking of SRP, we have a big issue with SRP in the SpitPlayerRules class. Right now SpitPlayerRules is both the test fixture and the game double. This is a clear violation of the principle. We've been aware of it for some time now. In fact it was a conscious decision to leave it alone, until now. We're going to extract the Game class from SpitPlayerRules. We're going to do this in very small steps and always keep the tests green because that's what the TDD people say to do. First I'll show the entire SpitPlayerRules, then I'll discuss each step and show the affected portions of the code.

class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @player = Player.new(self)
    #Game double's state
    @pile_one = SpitPile.new(7)
    @pile_two = SpitPile.new(0)
    @stock = []
    @spit_reserve = []
  end
  def test_run_without_draw
    @player.hand = [ 2, 3, 4, 5, 6 ]
    @player.play
    assert_equal [7, 6, 5, 4, 3, 2] , @pile_one.to_a
  end
  def test_player_draws_when_hand_is_short
    @stock << 2
    @player.hand = [3, 4, 5, 6]
    @player.play
    assert @stock.empty?, "player did not draw as expected"
  end
  def test_when_stuck_player_asks_for_new_spit
    @spit_reserve  << 3 << 6
    @player.hand = [2, 2, 3, 4, 5]
    @player.play
    assert @spit_reserve.empty?, "player did not ask for a new spit"
  end
  def test_player_can_play_on_either_pile
    @pile_two = SpitPile.new(5)
    @player.hand = [ 2, 3, 2, 3, 4 ]
    @player.play
    assert_equal [7] , @pile_one.to_a
    assert_equal [5, 4, 3, 2, 3, 2], @pile_two.to_a
  end

  # game methods
  def spit_on_one( card )
    @pile_one.play card
  end
  def spit_on_two( card )
    @pile_two.play card
  end
  def draw
    @stock.pop
  end
  def draw?
    ! @stock.empty?
  end 
  def stuck
    @pile_one.new_spit(@spit_reserve.pop)
  end 
end

First we need an empty class to move all the game methods to.

class Game
end

That was easy enough. And the tests are still green. Where do we go from here? Let's pick a method and move it to the new class. Remember we can't let the tests break so we'll need to give SpitPlayerRules an instance of Game to use. Sometimes it's easier to move methods and data in clumps. Let's move #draw, #draw?, and @stock.

class Game
  attr_accessor :stock
  def draw
    @stock.pop
  end
  def draw?
    ! @stock.empty?
  end
end
class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @player = Player.new(self)
    #Game double's state
    @game = Game.new
    @game.stock = []
    @pile_one = SpitPile.new(7)
    @pile_two = SpitPile.new(0)
    @spit_reserve = []
  end
  def test_player_draws_when_hand_is_short
    @game.stock << 2
    @player.hand = [3, 4, 5, 6]
    @player.play
    assert @game.stock.empty?, "player did not draw as expected"
  end
  # game methods
  def draw
    @game.draw
  end
  def draw?
    @game.draw?
  end 
end

Yes! The tests are still green. Alright all that's left is #stuck, #spit_on_*, @pile_*, and @spit_reserve. Let's move them.

class Game
  attr_reader :stock
  attr_reader :spit_reserve
  attr_accessor :pile_one
  attr_accessor :pile_two
  def spit_on_one( card )
    @pile_one.play card
  end
  def spit_on_two( card )
    @pile_two.play card
  end
  def stuck
    @pile_one.new_spit(@spit_reserve.pop)
  end 
end
class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @player = Player.new(self)
    #Game double's state
    @game = Game.new
    @game.stock = []
    @game.pile_one = SpitPile.new(7)
    @game.pile_two = SpitPile.new(0)
    @game.spit_reserve = []
  end
  def test_run_without_draw
    @player.hand = [ 2, 3, 4, 5, 6 ]
    @player.play
    assert_equal [7, 6, 5, 4, 3, 2] , @game.pile_one.to_a
  end
  def test_when_stuck_player_asks_for_new_spit
    @game.spit_reserve  [3, 6]
    @player.hand = [2, 2, 3, 4, 5]
    @player.play
    assert @game.spit_reserve.empty?, "player did not ask for a new spit"
  end
  def test_player_can_play_on_either_pile
    @game.pile_two = SpitPile.new(5)
    @player.hand = [ 2, 3, 2, 3, 4 ]
    @player.play
    assert_equal [7] , @game.pile_one.to_a
    assert_equal [5, 4, 3, 2, 3, 2], @game.pile_two.to_a
  end
  # game methods
  def spit_on_one( card )
    @game.pile_one.play card
  end
  def spit_on_two( card )
    @game.pile_two.play card
  end
  def stuck
    @game.pile_one.new_spit(@spit_reserve.pop)
  end 
end

Green Bar! Now let's move the game setup from SpitPlayerRules#setup to Game#initialize.

class Game
  def initialize
    @stock = []
    @spit_reserve = []
    @pile_one = SpitPile.new(7)
    @pile_two = SpitPile.new(0)
  end
end

Finally, we can replace the self shunt of SpitPlayerRules with @game, and remove all the game specific methods from SpitPlayerRules.

class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @game = Game.new
    @player = Player.new(@game)
  end
end

The tests are still green. One last thing, Game#initialize is setting up for the tests, and all of the data members of Game have accessors. Maybe we should introduce a GameDouble that will setup the state and provide access to the members. It might look like this

class GameDouble < Game
  attr_reader :stock
  attr_reader :spit_reserve
  attr_accessor :pile_one
  attr_accessor :pile_two
  def initialize
    @stock = []
    @spit_reserve = []
    @pile_one = SpitPile.new(7)
    @pile_two = SpitPile.new(0)
  end
end

I rather like that, so I'll hook it into the Player rules and clean up Game accordingly.

class SpitPlayerRules < Test::Unit::TestCase
  def setup 
    @game = GameDouble.new
    @player = Player.new(@game)
  end
end

Until Next Time

Whew. That was as lot of code movement. I'll show the code in it's entirety below. Let's look at what we've done today. We've finally broken out a Game class, we introduced a test double for that game class, and we brought the Player class one step closer to done. There are a few warts. The most notable wart is the integers that represent cards. We need to do something about that next time.