TDD Advanced Concepts : Angry Rock Kata

Objectives


  • What is Command Query Separation Principle?
  • How to fix Command Query Separation violation?
  • Refactoring : Retaining the old interface and the new one at the same time to avoid old tests from failing.
  • Semantic quirkiness of Well Grounded Rubyist book solution exposed by specs.
  • Using domain specific terms to make the code expressive

Difficulty Level


Medium

Command Query Separation Principle


The term 'command query separation' was coined by Bertrand Meyer in his book Object Oriented Software Construction. The fundamental idea is that we should divide an object's methods into two categories:

Queries: Return a result and do not change the observable state of the system (are free of side effects).
Commands: Change the state of a system but do not return a value.

Because the term 'command' is widely used in other contexts, Martin Fowler refers to it as 'modifiers' and 'mutators'. It's useful if you can clearly separate methods that change state from those that don't. This is because you can use queries in many situations with much more confidence, changing their order. You have to be careful with modifiers.

The return type is the give-away for the difference. It's a good convention because most of the time it works well. Consider iterating through a collection in Java: the next method both gives the next item in the collection and advances the iterator. It's preferable to separate advance and current methods. There are exceptions. Popping a stack is a good example of a modifier that modifies state. Meyer correctly says that you can avoid having this method, but it is a useful idiom. Follow this principle when you can.

Source : Command Query Separation by Martin Fowler.

Testing and Command Query Separation Principle


Tests are kept flexible when we follow this rule of thumb: Stub queries and expect commands, where a query is a method with no side effects that does nothing but query the state of an object and a command is a method with side effects that may, or may not, return a result. Of course, this rule does not hold all the time, but it's a useful starting point.

Source : jMock Home Page

Violation of Command Query Separation Principle


Steps


Step 1

Create angry_rock_spec.rb with the following contents:

require_relative 'angry_rock'

module Game
  describe AngryRock do
   it "should pick paper as the winner over rock" do
     choice_1 = Game::AngryRock.new(:paper)
     choice_2 = Game::AngryRock.new(:rock)
     winner = choice_1.play(choice_2)
     result = winner.move

     expect(result).to eq(:paper)
   end    
  end
end

Step 2

Create angry_rock.rb with the following contents:

module Game
  class AngryRock

  end
end

Step 3

Run the spec and watch it fail:

$rspec angry_rock_spec.rb --color --format doc

Game::AngryRock
  should pick paper as the winner over rock (FAILED - 1)
Failures:
  1) Game::AngryRock should pick paper as the winner over rock
     Failure/Error: choice_1 = Game::AngryRock.new(:paper)
     ArgumentError:
       wrong number of arguments(1 for 0)

Step 4

Let's get past this error by changing the angry_rock.rb as follows:

module Game
  class AngryRock
    def initialize(move)
      @move = move
    end
  end
end

Now we get the error:

1) Game::AngryRock should pick paper as the winner over rock
   Failure/Error: winner = choice_1.play(choice_2)
   NoMethodError:
     undefined method `play' for #<Game::AngryRock:0xb0 @move=:paper>

Step 5

So, let's define a empty play method as follows:

module Game
  class AngryRock
    # other code same as before
    def play

    end
  end
end

Step 6

Now we get:

1) Game::AngryRock should pick paper as the winner over rock
   Failure/Error: winner = choice_1.play(choice_2)
   ArgumentError:
     wrong number of arguments (1 for 0)

Step 7

Change the play method signature like this:

def play(other)

end

Step 8

Now we get:

1) Game::AngryRock should pick paper as the winner over rock
    Failure/Error: result = winner.move
    NoMethodError:
      undefined method `move' for nil:NilClass

Step 9

Change the play method like this:

def play(other)
  self
end

Step 10

1) Game::AngryRock should pick paper as the winner over rock
    Failure/Error: result = winner.move
    NoMethodError:
      undefined method `move' for #<Game::AngryRock:0xd8 @move=:paper>

Step 11

Change the angry_rock.rb as follows:

module Game
  class AngryRock
    attr_accessor :move

    # rest of the code same as before 
  end
end

Step 12

The first spec now passes. Add the second spec:

it "picks scissors as the winner over paper" do
  choice_1 = Game::AngryRock.new(:scissors)
  choice_2 = Game::AngryRock.new(:paper)
  winner = choice_1.play(choice_2)
  result = winner.move

  result.should == :scissors   
end

Step 13

It passes immediately. Make it fail by mutating the angry_rock.rb like this:

module Game
  class AngryRock
    # other code same as before
    def play(other)
      return other if other.move == :paper
      self
    end
  end
end

It now fails with :

1) Game::AngryRock picks scissors as the winner over paper
   Failure/Error: result.should == :scissors
     expected: :scissors
          got: :paper (using ==)

Step 14

Remove the short circuit statement:

return other if other.move == :paper 

from angry_rock.rb. The spec will now pass.

Step 15

Let's add the third spec:

it "picks rock as the winner over scissors " do
  choice_1 = Game::AngryRock.new(:rock)
  choice_2 = Game::AngryRock.new(:scissors)
  winner = choice_1.play(choice_2)
  result = winner.move

  expect(result).to eq(:rock)  
end

Step 16

This spec also passes without failing. Add a short circuit statement for the third spec and make the test fail and then make it pass again.

Step 17

Let's now add the spec for the tie case:

it "results in a tie when both players pick rock" do
  choice_1 = Game::AngryRock.new(:rock)
  choice_2 = Game::AngryRock.new(:rock)
  winner = choice_1.play(choice_2)
  result = winner.move

  winner.should be_false
end

Step 18

This fails with the error:

1) Game::AngryRock results in a tie when both players pick rock
   Failure/Error: winner.should be_false
     expected: false value
          got: #<Game::AngryRock:0xc0 @move=:rock>

Step 19

Change the implementation of play method like this:

def play(other)
  return false if self.move == other.move
  self
end

Discussion


Now all the specs will pass. This implementation of play method is a lousy design. The false case breaks the consistency of the returned value and violates the semantics of the API. Also the play method is a “Command” not a “Query”. This method violates the Command Query Separation Principle.

alt text

Fixing the Bad Design


Step 1

Change the spec for tie case to:

it "results in a tie when both players pick rock" do
  choice_1 = Game::AngryRock.new(:rock)
  choice_2 = Game::AngryRock.new(:rock)
  winner = choice_1.play(choice_2)
  result = winner.move

  expect(result).to eq("TIE!") 
end

Step 2

This fails with the error:

1) Game::AngryRock results in a tie when both players pick rock
   Failure/Error: result = winner.move
   NoMethodError:
     undefined method `move' for false:FalseClass

Step 3

Change the implementation of the play method as follows:

def play(other)
  return AngryRock.new("TIE!") if self.move == other.move
  self
end

Step 4

Now all specs pass. The play method now returns a AngryRock tie object for the tie case.

Step 5

Add two more specs for the remaining tie cases one by one.

it "results in a tie when both players pick paper" do
  choice_1 = Game::AngryRock.new(:paper)
  choice_2 = Game::AngryRock.new(:paper)
  winner = choice_1.play(choice_2)
  result = winner.move

  expect(result).to eq("TIE!")      
end

it "results in a tie when both players pick scissors" do
  choice_1 = Game::AngryRock.new(:scissors)
  choice_2 = Game::AngryRock.new(:scissors)
  winner = choice_1.play(choice_2)
  result = winner.move

  expect(result).to eq("TIE!")      
end

Step 6

Make them fail and then make it pass one by one. The last three specs show three possible tie scenarios.

Removing the Duplication in Specs


The Before Picture

it "results in a tie when the same choice is made by both players" do
  [:rock, :paper, :scissors].each do |choice|
    choice_1 = Game::AngryRock.new(choice)
    choice_2 = Game::AngryRock.new(choice)
    winner = choice_1.play(choice_2)
    result = winner.move

    expect(result).to eq("TIE!")      
  end     
end   

The duplication in specs is removed by using a loop. We can do better than that, let's apply what we learned in Eliminating Loops chapter.

The After Picture

Step 1

Replace the loop version of the tie case with the following spec:

it "results in a tie when the same choice is made by both players" do
  data_driven_spec([:rock, :paper, :scissors]) do |choice|
    choice_1 = Game::AngryRock.new(choice)
    choice_2 = Game::AngryRock.new(choice)
    winner = choice_1.play(choice_2)
    result = winner.move

    expect(result).to eq("TIE!")      
  end     
end   

Step 2

Add a helper method in spec_helper.rb:

def data_driven_spec(container)
  container.each do |element|
   yield element
  end
end

Step 3

Add :

require_relative 'spec_helper'

to the top of the angry_rock_spec.rb.

Step 4

Now all the specs should still pass. Let's now improve the design.

Step 5

To make the specs more readable change specs and production code as follows:

require_relative 'angry_rock'
require_relative 'spec_helper'

module AngryRock
  describe Choice do
   it "picks paper as the winner over rock" do
     choice_1 = AngryRock::Choice.new(:paper)
     choice_2 = AngryRock::Choice.new(:rock)
     winner = choice_1.play(choice_2)
     result = winner.move

     expect(result).to eq(:paper)      
   end    
   it "picks scissors as the winner over paper" do
     choice_1 = AngryRock::Choice.new(:scissors)
     choice_2 = AngryRock::Choice.new(:paper)
     winner = choice_1.play(choice_2)
     result = winner.move

     expect(result).to eq(:scissors)      
   end
   it "picks rock as the winner over scissors " do
     choice_1 = AngryRock::Choice.new(:rock)
     choice_2 = AngryRock::Choice.new(:scissors)
     winner = choice_1.play(choice_2)
     result = winner.move

     expect(result).to eq(:rock)  
   end
   it "results in a tie when the same choice is made by both players" do
     data_driven_spec([:rock, :paper, :scissors]) do |choice|
       choice_1 = AngryRock::Choice.new(choice)
       choice_2 = AngryRock::Choice.new(choice)
       winner = choice_1.play(choice_2)
       result = winner.move

       expect(result).to eq("TIE!")
     end     
   end   
  end
end

module AngryRock 
  class Choice
    attr_accessor :move

    def initialize(move)
      @move = move
    end
    def play(other)
      return Choice.new("TIE!") if self.move == other.move
      self
    end    
  end
end

The specs should still pass. The specs now read well and make much more sense than the previous version. Note: Don't make changes both to production and test code at the same time without running the tests. Unless you have a good reason, we always run tests after we modify either the specs or the production code.

Violation of Command Query Separation Principle


Is the play() method a command or a query? It is ambiguous because play() seems to be a name of a command and it is returning the winning AngryRock object (result of a query operation). It combines command and query.

Steps


Step 1

Let's refactor while we stay green. What if the specs that we wrote was intelligent enough to use CQS principle like this:

require_relative 'angry_rock'
require_relative 'spec_helper'

module AngryRock
  describe Game do
   it "should pick paper as the winner over rock" do
     game = AngryRock::Game.new(:paper, :rock)
     game.play
     winning_move = game.winning_move

     expect(winning_move).to eq(:paper)
   end
end

Step 2

We create a game object by providing two choices, we play the game by using the command method play and we query the winning_move. Then we make our assertion on the winning move. To make this spec pass, change the implementation of the game as follows:

module AngryRock 
  class Game
    WINS = {rock: :scissors, scissors: :paper, paper: :rock}

    def initialize(choice_1, choice_2)
      @choice_1 = choice_1
      @choice_2 = choice_2
    end
    def play
      @winner = winner
    end
    def winning_move
      @winner
    end
    def winner
      if WINS[@choice_1]
        @choice_1
      else
        @choice_2
      end
    end
  end  
end

Step 3

Let's add the second and third specs and make sure they pass:

it "picks scissors as the winner over paper" do
  game = AngryRock::Game.new(:scissors, :paper)

  game.play

  winning_move = game.winning_move  
  expect(winning_move).to eq(:scissors)
end

it "picks rock as the winner over scissors " do
  game = AngryRock::Game.new(:rock, :scissors)

  game.play

  winning_move = game.winning_move  
  expect(winning_move).to eq(:rock)
end

Step 4

Let's add the tie case spec:

it "results in a tie when the same choice is made by both players" do
  data_driven_spec([:rock, :paper, :scissors]) do |choice|
    game = AngryRock::Game.new(choice, choice)

    game.play

    winning_move = game.winning_move   
    expect(winning_move).to eq(:tie)    
  end     
end

Step 5

To make it pass change the production code as follows:

def winner
  return :tie if @choice_1 == @choice_2
  if WINS[@choice_1]
    @choice_1
  else
    @choice_2
  end
end

Step 6

Now all specs should pass. Now the play() is a command and winner() is a query. The command and query are now separated and the code obeys the CQS principle.

Handling Illegal Inputs


Steps


Step 1

Let's make the code robust by checking for illegal inputs. Add the following spec:

it "should raise exception when illegal input is provided" do
  expect do
    game = AngryRock::Game.new(:punk, :hunk)
    game.play
  end.to raise_error
end

Step 2

It fails with the following error:

1) AngryRock::Game should raise exception when illegal input is provided
    Failure/Error: expect do
      expected Exception but nothing was raised

Step 3

To make this spec pass change the winner method like this:

def winner
  return :tie if @choice_1 == @choice_2
  if WINS.fetch(@choice_1)
    @choice_1
  else
    @choice_2
  end
end

Step 4

All specs will now pass.

Step 5

Let's hide the implementation details by making the winner method private. All specs should still pass.

Step 6

Let's make the exception user friendly, change the illegal input spec as follows:

it "should raise exception when illegal input is provided" do
  expect do
    game = AngryRock::Game.new(:punk, :hunk)
    game.play
  end.to raise_error(IllegalChoice)
end

Step 7

This fails with the error:

1) AngryRock::Game should raise exception when illegal input is provided
   Failure/Error: end.to raise_error(IllegalChoice)
   NameError:
     uninitialized constant AngryRock::IllegalChoice

Step 8

Change the angry_rock.rb as follows:

module AngryRock 
  class IllegalChoice < Exception ; end;

  class Game
   # rest of the code is same as before 
    def winner
      return :tie if @choice_1 == @choice_2
      begin
        if WINS.fetch(@choice_1)
          @choice_1
        else
          @choice_2
        end
      rescue
        raise IllegalChoice
      end
    end
  end  
end

Step 9

All specs now pass. This concise solution is based on Sinatra Up and Running By Alan Harris, Konstantin Haase.

Exercise


  1. Look at the following alternative specs and implementation. This solution is based on Well Grounded Rubyist by David Black. It has been refactored to a better design. Make it even better by making the code expressive with readable specs. Compare your solution with the solutions given in the appendix of Essential TDD book book.

Here is the angryrockspec.rb

require 'spec_helper'

module Game
  describe AngryRock do
   it "should pick paper as the winner over rock" do
     play = Play.new(:paper, :rock)

     play.should have_winner
     expect(play.winning_move).to eq("paper")
   end 
   it "picks scissors as the winner over paper" do
     play = Play.new(:scissors, :paper)

     play.should have_winner
     expect(play.winning_move).to eq("scissors")  
   end   
   it "picks rock as the winner over scissors " do
     play = Play.new(:rock, :scissors)

     play.should have_winner
     expect(play.winning_move).to eq("rock")       
   end
   it "results in a tie when the same choice is made by both players" do
     data_driven_spec([:rock, :paper, :scissors]) do |choice|
       play = Play.new(choice, choice)

       play.should_not have_winner
     end     
   end   
  end
end

Here is the angry_rock.rb

module Game
  class AngryRock
    include Comparable

    WINS = [ %w{rock scissors}, %w{scissors paper}, %w{paper rock}]

    attr_accessor :move

    def initialize(move)
      @move = move.to_s
    end
    def <=>(opponent)
      if move == opponent.move
        0
      elsif WINS.include?([move, opponent.move])
        1
      elsif WINS.include?([opponent.move, move])
        -1
      else
        raise ArgumentError, "Something's wrong"
      end
    end
    def winner(opponent)
      if self > opponent
        self
      elsif opponent > self
        opponent
      end
    end
  end

  class Play
    def initialize(first_choice, second_choice)
      choice_1 = AngryRock.new(first_choice)
      choice_2 = AngryRock.new(second_choice)

      @winner = choice_1.winner(choice_2)
    end
    def has_winner?
      !@winner.nil?
    end
    def winning_move
      @winner.move
    end
  end
end  


Related Articles


Create your own user feedback survey