Spoj's 1st problem in tdd using Ruby

Question

I am pretty new to TDD and is following problems on spoj.pl for practice. I need help with following code; it was my first attempt.

Problem: Your program is to use the brute-force approach in order to find the Answer to Life, the Universe, and Everything. More precisely... rewrite small numbers from input to output. Stop processing input after reading in the number 42. All numbers at input are integers of one or two digits. Mentioned here.

Tests:

describe "life universe and everything" do

 before(:each) do 
  @life = Life.new
 end

 it "should accept input" do 
  @life.stub(:gets) { "5, 4, 23, 42,  4" }
  @life.input.should == [5,4,23,42,4]
 end

 it "check number validity" do
   @life.valid?(23).should be_true
   @life.valid?(42).should be_false
 end

 it "process input" do
   array1 = [5,4,23,42,4]
   @life.process_input(array1).should == [5,4,23]
   array2 = [1,4,76,34,90]
 @life.process_input(array2).should == [1,4,76,34,90]
end

end

Code:

class Life

 def input
  puts "List comma(',') seperated numbers. Press enter when done."
  numbers_string = gets
  numbers = numbers_string.split(',')
  numbers.map {|n| n.strip.to_i}
 end

 def valid?(number)
  (number==42) ? false : true
 end

 def process_input(numbers)
  processed_numbers = []
  numbers.each do |number|
  break unless valid?(number)
  processed_numbers << number
 end
  processed_numbers
end

end

I would like it to be reviewed and would be glad to know my mistakes and what can I do to improve quality of code. Thanks!

Answer 1

Congratulations. That's a pretty nice first attempt.

I see only a few issues:

• The problem statement shows that you will receive one number per line; your code expects one line with multiple numbers, separated by commas.

• The problem statement asks you to print the numbers; your program doesn't.

• When run, the test writes its prompt to $stdout. Tests should be quiet when passing.

The issue with the prompt can be fixed with an expectation:

@life.should_receive(:puts)\
.with("List comma(',') separated numbers.  Press enter when done"

If your intent is to solve a slightly different problem, then we won't worry about the differences between what your program does and what they're asking for. But read below the fold for a simple test and implementation, TDD Style, for the stated problem.

---

This problem can be solved pretty simply. For example,

  loop do
    n = gets.chomp.to_i
    puts n
    break if n == 42
  end

What test or tests could you write that would allow you to have code this simple? Let's see if we can get there from nothing:

class Life
  def process_input
  end
end

describe Life do

  it "should copy numbers from $stdin to $stdout" do
    life = Life.new
    life.should_receive(:gets).with(no_args).and_return('41')
    life.should_receive(:puts).with(41)
    life.process_input
  end

end

Running that, we find that the spec fails because the code never called gets. We add the call to gets, then find that the spec fails because the code never called puts. Adding them, we get a passing test:

class Life
  def process_input
    puts gets.to_i
  end
end

Of course, we've got no loop, so let's tell the test there should be multiple calls to gets and puts. We'll use .ordered to tell rspec that the calls should occur in a certain order.

it "should copy numbers from $stdin to $stdout" do
  life = Life.new
  life.should_receive(:gets).ordered.with(no_args).and_return('41')
  life.should_receive(:puts).ordered.with(41)
  life.should_receive(:gets).ordered.with(no_args).and_return('42')
  life.should_receive(:puts).ordered.with(42)
  life.process_input
end

Now there needs to be a loop. Let's add it:

def process_input
  loop do
    puts gets.to_i
  end
end

Our test neither passes nor fails. Instead, it hangs. That's because we didn't give it any way to get out of the loop. Let's add the termination condition:

def process_input
  loop do
    n = gets.to_i
    puts n
    break if n == 42
  end
end

So, we can write a test that lets us have pretty simple code. But in a few ways, this test is bothersome. One reason is that it is too picky about the way that the I/O is done. What if we changed the program to use print instead of puts? It would have produce exactly the same output, but the test would fail. Also, having the test hang when the program fails to terminate isn't very good. We want tests that pass or fail quickly and cleanly, with no guessing. So let's rewrite our test (and the program) using dependency injection for I/O.

---

Starting over, we're going to change the the Life program so that it takes and input object and an output object:

class Life

  def initialize(input = $stdin, output = $stdout)
    @input = input
    @output = output
  end

  def process_input
  end

end

And our test:

require 'stringio'

describe Life do

  it "should copy numbers from input to output" do
    input = StringIO.new("41\n")
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n"
  end

end

Using StringIO instances as mock I/O objects is very handy, and lets us test the result of the I/O rather than the actions that produce the I/O. That's nice.

Since our program doesn't process anything yet, the test fails:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: output.string.should == "41\n"
       expected: "41\n"
            got: "" (using ==)

Let's fix that:

  def process_input
    @output.print @input.gets
  end

The test now passes. Let's modify the test to show that it should loop:

  it "should copy numbers from $stdin to $stdout" do
    input = StringIO.new("41\n42\n")
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n42\n"
  end

The test fails:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: output.string.should == "41\n42\n"
       expected: "41\n42\n"
            got: "41\n" (using ==)

Let's add the loop:

  def process_input
    loop do
      @output.print @input.gets
    end
  end

Now it hangs again! That's because I/O objects like StringIO and $stdin just return a nil when they run out of input. Our program is happily printing nil over and over. Let's make the test die nicely when the loop fails to terminate:

  it "should copy numbers from $stdin to $stdout" do
    input = StringIO.new("41\n42\n43\n")
    def input.gets
      super.tap { |s| raise "no more input" unless s }
    end
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n42\n"
  end

Here we monkey patch our input objects so that when it runs out of input, it raises an error rather than just returning nil. The result:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: super.tap { |s| raise "no more input" unless s }
     RuntimeError:
       no more input

Great! let's get the loop to terminate when it encounters that "42":

  def process_input
    loop do
      s = @input.gets
      @output.print s
      break if s == "42\n"
    end
  end

Now we've got a robust test that doesn't care about the details of how I/O is done, and code that is pretty simple.

---

There's just one more enhancement I'd make to this program, and that is to have it exit when it runs out of input. While not strictly required by the problem statement, a program that prints blank lines endlessly upon EOF is obnoxious! Now we need another "it" in our test:

  it "should exit on EOF" do
    input = StringIO.new("41\n<EOF>")
    def input.gets
      s = super
      case s
      when "<EOF>"
        nil
      when nil
        raise "no more input"
      else
        s
      end
    end
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n"
  end

Our monkey-patched input object just got a little more interesting. We want it to return "nil" on end of input, because that's how the loop will detect end of input in real life, but we want it to raise an exception if the code under test tries to get more input after the nil. This test fails:

  1) Life should exit on EOF
     Failure/Error: raise "no more input"
     RuntimeError:
       no more input

So let's make the loop exit on end of input:

  def process_input
    loop do
      s = @input.gets
      @output.print s
      break if s.nil?
      break if s == "42\n"
    end
  end

Now the program meets the stated requirements, and our own requirement of not filling the output with blank lines on end-of-input.

Answer 2

This wouldn't be the way I'd develop it, your class is used more as a module (a library of functions) and not in OOP, ie: you're not using instance variables. I'd rather go for something like the following:

class Life

  def input
    puts "List comma (',') seperated numbers. Press enter when done."
    @data    = user_string
    @numbers = input_to_numbers!
  end

  # NOTE: I don't know how to stub instance variables in rspec
  # so I stub these two methods
  def numbers
    @numbers
  end

  def user_string
    gets
  end

  # NOTE: this works similarly to
  # numbers.first numbers.index(42) || numbers.size
  def process_input
    processed_numbers = []
    numbers.each do |number|
      break unless valid?(number)
      processed_numbers << number
    end
    processed_numbers
  end


private

  def input_to_numbers!
    @data.split(',').map(&:to_i)
  end

  def valid?(number)
    number != 42
  end

end

and I'd test something like this:

require './code1.rb'

describe "life universe and everything" do
  before(:each) do 
    @life = Life.new
  end

  it "should accept input" do 
    @life.stub(:user_string) { "5, 4, 23, 42,  4" }
    @life.input.should == [5,4,23,42,4]
  end

  # NOTE: many people wouldn't test private methods
  # saying you should only test the functionality you expose
  # I personally see nothing wrong
  it "check number validity" do
    @life.send(:valid? , 23).should be_true
    @life.send(:valid? , 42).should be_false
  end

  it "process input without 42" do
    @life.stub(:numbers) {[5,4,23,42,4]} 
    @life.process_input.should == [5,4,23]
  end

  it "process input with 42" do
    @life.stub(:numbers) {[1,4,76,34,90]}
    @life.process_input.should == [1,4,76,34,90]
  end
end

I'm not sure I got what you mean, but I hope it's useful.