Ruby Kata Gem: Calculator

Question

I've just completed the Calculator Kata in the Kata Gem and I was hoping for some feedback. Here's the Kata:

Calculator Kata
 Create a calculator that is initialized with a string expression
  - detail: The expression is of the form digits separated by commas: "1,2"
  - detail: The expression is accessed by a method named expr
  - detail: The expression can be reset for evaluation at any time without re-initializing
  - example: Calculator.new "1,2"

completed (Y|n): y

   Add Method
    Create an add method that sums the string expression
     - detail: The method will return the sum of the digits
     - detail: The expression can contain 0, 1 or 2 numbers
     - detail: Then empty string will return 0
     - example: "" computes to 0
     - example: "1" computes to 1
     - example: "1,2" computes to 3

completed (Y|n):

  Allow the expression to contain an unknown amount of numbers
     - example: "1,2,3" computes to 6
     - example: "1,2,5,8" computes to 16

completed (Y|n):

Diff Method
  Create a diff method that computes the consecutive differences
     - detail: The expression must contain at least 2 digits
     - example: "1,0" compues to 1
     - example: "3,2,1" computes to 0
     - example: "5,4,3,2,1" computes to -5
     - detail: Expressions with less than 2 digits raise an exception
     - example: "" or "5"

completed (Y|n):

Prod Method
  Create a prod method that computes the multiples in the expression
     - detail: The method will return the product of the numbers
     - example: "0" computes to 0
     - example: "2,1" computes to 2
     - example: "3,2,1" computes to 6

completed (Y|n):

Div Method
  Create a div method that computes the consecutive divisions in the expression
     - detail: The method will return the final quotient of the numbers
     - detail: it will raise an exception if the expression contains the number 0
     - example: "2,1" computes to 2
     - example: "3,2,1" computes to 1
     - example: "1,2,3" computes to 0

completed (Y|n):



Congratulations!
- Create a calculator that is initialized with a string expression         01:15:02
- Create an add method that sums the string expression                     14:34:41
- Allow the expression to contain an unknown amount of numbers             00:04:33 
- Create a diff method that computes the consecutive differences           00:06:14
- Create a prod method that computes the multiples in the expression       00:20:07
- Create a div method that computes the consecutive divisions in the exp   00:11:54
----------------------------------------------------------------------     --------
Total Time taking Calculator kata:                                         16:32:31

And, here is my solution (specs and the Calculator class are in the same file):

class Calculator
  def initialize(expression="")
    @expression = expression.split(",").map(&:to_i) || 0
    if [email protected]?
      @expression = [0]
    end
  end

  def expr
    @expression
  end

  def sum
    @expression.inject(&:+) || 0
  end

  def diff
    if @expression.compact.count < 2
      raise "expect 2 or more elements but recieved #{@expression.compact.count}"
    end
    @expression.inject(&:-)
  end

  def prod
    @expression.inject(&:*)
  end

  def div
    if @expression.find { |element| element == 0 }
      raise "divide by zero"
    end
    @expression.inject(&:/)
  end
end

describe "Calculator" do
  subject(:calculator) { Calculator.new(expression) }

  shared_examples "methods" do
    specify { expect { calculator }.to_not raise_exception }

    it { should respond_to(:expr) }
    it { should respond_to(:sum)  }
    it { should respond_to(:diff) }
    it { should respond_to(:prod) }
    it { should respond_to(:div)  }
  end

  shared_examples "too few elements" do
    specify { expect { calculator.diff }.to raise_exception }
  end

  shared_examples "divide by zero" do
    specify { expect { calculator.div }.to raise_exception }
  end

  context "with argument ''" do
    let(:expression) { "" }

    it_behaves_like "methods"
    it_behaves_like "too few elements"
    it_behaves_like "divide by zero"

    its(:expr) { should eq([0]) }
    its(:sum)  { should eq(0)   }
    its(:prod) { should eq(0)   }
  end

  context "with argument '1'" do
    let(:expression) { "1" }

    it_behaves_like "methods"
    it_behaves_like "too few elements"

    its(:expr) { should eq([1]) }
    its(:sum)  { should eq(1)   }
    its(:prod) { should eq(1)   }
  end

  context "with argument '1,2'" do
    let(:expression) { "1,2" }

    it_behaves_like "methods"

    its(:expr) { should eq([1,2]) }
    its(:sum)  { should eq(3)     }
    its(:diff) { should eq(-1)    }
    its(:prod) { should eq(2)     }
  end

  context "with argument '1,2,3," do
    let(:expression) { "1,2,3" }

    it_behaves_like "methods"

    its(:expr) { should eq([1,2,3]) }
    its(:sum)  { should eq(6)       }
    its(:diff) { should eq(-4)      }
    its(:prod) { should eq(6)       }
    its(:div)  { should eq(0)       }
  end

  context "with argument '1,2,5,8'" do
    let(:expression) { "1,2,5,8" }

    it_behaves_like "methods"

    its(:expr) { should eq([1,2,5,8]) }
    its(:sum)  { should eq(16)        }
    its(:diff) { should eq(-14)       }
    its(:prod) { should eq(80)        }
  end

  context "with argument '1,0'" do
    let(:expression) { "1,0" }

    it_behaves_like "divide by zero"

    its(:diff) { should eq(1) }
    its(:prod) { should eq(0) }
  end

  context "with argument '3,2,1'" do
    let(:expression) { "3,2,1" }

    its(:diff) { should eq(0)   }
    its(:prod) { should eq(6)   }
    its(:div)  { should eq(1) }
  end

  context "with argument '5,4,3,2,1,'" do
    let(:expression) { "5,4,3,2,1" }

    its(:diff) { should eq(-5)  }
    its(:prod) { should eq(120) }
  end

  context "with arguments '2,1'" do
    let(:expression) { "2,1" }

    its(:div) { should eq(2) }
  end
end

Answer 1

Looks good! Most importantly, you're using inject properly. The specs seem nice too (although running all the "methods" expectations every time is maybe a little redundant).

But I don't see support for this requirement:

The expression can be reset for evaluation at any time without re-initializing

Right now, you only have a reader method for expr.
Also, that reader returns an array - not the original string. While it's not stated directly, I think it's implied that you should get a string back. calculator.expr = calculator.expr should be idempotent.

There's also something weird going on in your current initialize method:

@expression = expression.split(",").map(&:to_i) || 0

I don't see how it would ever get to the OR (split will either give you an empty array or raise an exception), but even if it did, it'd just raise an exception in the next line:

if [email protected]?

since Fixnum doesn't respond to any?.

But it wouldn't take much to fix all of this. Something like this should be OK

class Calculator
  attr_reader :expr

  def expr=(string='')
    @expr = string
    @expression = @expr.split(",").map(&:to_i)
    @expression = [0] if @expression.none?
  end

  alias_method :initialize, :expr=

  #...
end

But I'd probably rename @expression to @values or something, just to make the separation clearer (and to give the array a pluralized name).

The div method can be made simpler, as this

 if @expression.find { |element| element == 0 }

can be expressed as just:

 if @expression.include?(0)

And lastly, you may want to raise the built-in ZeroDivisionError instead of a custom one:

 raise ZeroDivisionError if @expression.include?(0)