2
\$\begingroup\$

This is the CalculatorAdvanced Kata from the Kata gem. I've just finished it and would love some feedback on my solution.

CalculatorAdvanced Kata
   Add Method
      Allow the expression to handle new lines between numbers
         - example: "1\n2\n3" computes to 6
         - example: "2,3\n4" computes to 9
         - detail: Consecutive use of delimeters should raise an exception
         - example: "1,\n2" or "1\n,2"

completed (Y|n):

      Calling method with a negative number will give an exception
         - detail: The exception should tell the user "negatives not allowed"
         - detail: The exception will list the negative number that was in the string
         - detail: The exception should list all negatives if there is more than one

completed (Y|n):

   Diff and Product Method
      should raise the same exceptions as the add method
         - detail: Consecutive Delimiters
         - detail: Negative Numbers

completed (Y|n):

   Define Custom Delimeters
      Allow the add method to accept a different delimiter
         - detail: The line of the string will contain "//[delimeter]\n...
         - detail: This line is optional and all previous tests should pass
         - example: "//[;]\n1;2" computes to 3
         - detail: "1;2" should raise an exception

completed (Y|n):

      Allow the diff method to accept a different delimiter like add
         - example: //[;]\n2;1 computes to 1

completed (Y|n):

      Allow the prod method to accept a different delimiter like add
         - example: //[;]\n2;1 computes to 2

completed (Y|n):

      Allow the div method to accept a different delimiter like add
         - example: //[;]\n3;2 computes to 1

completed (Y|n):

      Allow the add method to handle multiple different delimeters
         - example: multiple delimeters can be specified using "//[delimeter]...[delimeter]\n...
         - example: "//[*][;]\n1*2;3" computes to 6
         - example: "//[*][;][#]\n5*4;3#2" computes to -4
         - example: "//[#][;][*]\n1*2#3;4,5\n6" computes to 21

completed (Y|n):

      Allow the diff method to handle multiple different delimeters
         - example: "//[*][;]\n1*2;3" computes to -4

completed (Y|n):

      Allow the prod method to handle multiple different delimeters
         - example: "//[*][;]\n1*2;3" computes to 6

completed (Y|n):

      Allow the div method to handle multiple different delimeters
         - example: "//[*][;]\n1*2;3" computes to 0

completed (Y|n):

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┓
┃ Requirement                                                                      ┃ Time     ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the expression to handle new lines between numbers                         ┃ 00:37:20 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Calling method with a negative number will give an exception                     ┃ 00:20:55 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ should raise the same exceptions as the add method                               ┃ 00:08:41 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the add method to accept a different delimiter                             ┃ 00:22:51 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the diff method to accept a different delimiter like add                   ┃ 00:06:04 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the prod method to accept a different delimiter like add                   ┃ 00:01:49 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the div method to accept a different delimiter like add                    ┃ 00:01:46 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the add method to handle multiple different delimeters                     ┃ 00:55:48 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the diff method to handle multiple different delimeters                    ┃ 00:00:54 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the prod method to handle multiple different delimeters                    ┃ 00:00:38 ┃
┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╊━━━━━━━━━━┫
┃ Allow the div method to handle multiple different delimeters                     ┃ 00:00:43 ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┻━━━━━━━━━━┛

And, here's the Class I created:

class Calculatoradvanced
  attr_reader :expr

  def expr=(expression='')
    @expr = expression
    validate_expression
  end

  alias_method :initialize, :expr=

  def add
    @values.inject(&:+)
  end

  def diff
    @values.inject(&:-)
  end

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

  def div
    @values.inject(&:/)
  end

  private

  def validate_expression
    if @expr =~ /,\n/ || @expr =~ /\n,/
      raise "Consecutive Delimiters"
    end

    split_string = ",\n"

    if match = @expr.match(/\/\/([^\n]*)\n(.*)/m)
      delimeters, @expr = match.captures
      delimeters = delimeters.split(/[\[\]]/)
      delimeters.reject! { |delimeter| delimeter == '' }
      split_string = "#{Regexp.escape(delimeters.join)}" + split_string
    end

    @values = @expr.split(/[#{split_string}]/).map(&:to_i)

    @negatives = @values.select { |value| value < 0 }
    if (@negatives && @negatives.any?)
      raise "negatives not allowed: #{@negatives.join(', ')}"
    end
  end
end

I'm really unsatisfied with the test for Consecutive Delimiters. If anyone knows of a good way to make that test more flexible, I'd love to hear it.

Here are my specs:

require 'spec_helper'
require 'calculatoradvanced'

describe Calculatoradvanced do
  subject(:calc) { Calculatoradvanced.new(expression) }
  context 'with an empty expression' do
    let(:expression) { '' }
    specify { expect { calc }.to_not raise_exception }
  end
  context 'with expression "1\n2\n3"' do
    let(:expression) { "1\n2\n3" }

    specify { expect { calc }.to_not raise_exception }
    its(:add) { should eq(6) }
  end
  context 'with expression "2,3\n4"' do
    let(:expression) { "2,3\n4" }

    its(:add) { should eq(9) }
    its(:expr) { should eq("2,3\n4") }
  end
  context 'with expression "1,\n2"' do
    let(:expression) { "1,\n2" }

    specify { expect { calc.add }.to raise_exception }
  end
  context 'with expression "1\n,2"' do
    let(:expression) { "1\n,2" }

    specify { expect { calc.add }.to raise_exception }
  end
  shared_examples_for 'expression validation' do
    specify { expect { method }.to raise_exception(RuntimeError, /negatives not allowed/) }
    specify { expect { method }.to raise_exception(RuntimeError, /-1, -2, -3/) }
  end
  context 'with expression "-1,-2\n-3"' do
    let(:expression) { "-1,-2\n-3" }
    context '.add' do
      let(:method) { calc.add }

      it_behaves_like 'expression validation'
    end
    context '.diff' do
      let(:method) { calc.diff }
      it_behaves_like 'expression validation'
    end
    context '.prod' do
      let(:method) { calc.prod }
      it_behaves_like 'expression validation'
    end
  end
  context 'with expression "//[;]\n1;2"' do
    let(:expression) { "//[;]\n1;2" }

    its(:add) { should eq(3) }
  end
  context 'with expression "//[;]\n2;1"' do
    let(:expression) { "//[;]\n2;1" }
    its(:diff) { should eq(1) }
    its(:prod) { should eq(2) }
  end
  context 'with expression "//[;]\n3;2"' do
    let(:expression) { "//[;]\n3;2" }

    its(:div) { should eq(1) }
  end
  context 'with expression "//[*][;]\n1*2;3"' do
    let(:expression) { "//[*][;]\n1*2;3" }
    its(:add)  { should eq(6)  }
    its(:diff) { should eq(-4) }
    its(:prod) { should eq(6)  }
    its(:div)  { should eq(0)  }
  end
  context 'with expression "//[*][;][#]\n5*4;3#2"' do
    let(:expression) { "//[*][;][#]\n5*4;3#2" }
    its(:diff) { should eq(-4) }
  end
  context 'with expression "//[#][;][*]\n1*2#3;4,5\n6"' do
    let(:expression) { "//[#][;][*]\n1*2#3;4,5\n6" }
    its(:add) { should eq(21) }
  end
end
\$\endgroup\$
2
\$\begingroup\$

I am not going to re-write your code but give some pointers instead, I hope it's helpful:

  • Those four methods (add, ...) do basically the same thing, why don't you abstract them?
  • inject accepts a symbol: @values.inject(:+)
  • split_string -> maybe this? string.split(/[,$]/)
  • Variable delimeters has three different values on the course of the method! Different values deserve different names.
  • if @negatives: That's an unnecessary check, @negatives is an array, so it will never be falsy (in Ruby: nil/false).
  • @negatives.any? If you are going to perform an any? operation, do it directly (with a block), don't do a select + any?.
\$\endgroup\$
  • \$\begingroup\$ thanks for the feedback. I'll try using the old method_not_found trick and then having a case statement for add, etc... I'm a bit prejudice against that pattern but ... this is just for fun so why not make sure I understand that pattern before rejecting it. Good catch on inject(). Re: split_string, it actually does need to be "\n" and not "$". The newline character can occur multiple times in the string. delimiters is also an interesting case. I only want to final value so I'm stripping away unwanted characters in each line. I could try chaining the whole thing together ... \$\endgroup\$ – user341493 Nov 15 '13 at 17:14
  • \$\begingroup\$ -- continued :) -- Re: @negatives, the reason I'm checking @negatives && @negatives.any? is that, if there are no negative numbers, @negatives will be nil. This will cause @negatives.any? to throw an error because any? isn't defined on the Nil class. Also, I'm using select to get an array of all negatives because I need to print all negative numbers in the exception. I hope that clarifies what I was thinking with the choices I've made. Please let me know if I'm missing something and there's a better way to satisfy the requirements. Thanks! \$\endgroup\$ – user341493 Nov 15 '13 at 17:19
  • \$\begingroup\$ Re: Re: @negatives, you're right! select will return the empty array if it doesn't find anything so @negatives.any? is the only test that's needed there :) \$\endgroup\$ – user341493 Nov 15 '13 at 17:27
  • \$\begingroup\$ please don't use method_not_found! \$\endgroup\$ – tokland Nov 15 '13 at 18:20
  • \$\begingroup\$ lulz ... I know, I felt dirty doing it ;) What would you suggest to get the same effect (i.e. a case statement that maps the method to a symbol to pass to inject)? \$\endgroup\$ – user341493 Nov 15 '13 at 18:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.