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)

• user341493: Thank you so much for your feedback. This is really very helpful.
• Flambino: @user341493 No prob. Don't forget to click the checkmark to mark your question as answered (no rush though)