2
\$\begingroup\$

Is there a more cleaner approach to this? I'm doing the String calculator Kata posted by Roy Osherove. I realized that my calculator has no way of clearing out a previous expression after calling add. How can I handle this, and also how can I cut down the number of methods?

Here's a valid string: calc.add("//+\n1+2")

Here's an invalid string: calc.add("//b\n1b2")

class StringCalculator
  attr_reader :numbers

  def initialize(numbers = '')
    @numbers = numbers
  end

  def add(expression)
    @numbers << expression
    return nil if @numbers.end_with("\n")
    @numbers.gsub!(/\n/, delimiter)
    @numbers.empty? ? 0 : solution
  end

  private

  def valid_delimiters
    [*(33..46), *(58..64)].map(&:chr).join
  end

  def solution
    numerics = @numbers.split(delimiter).map(&:to_i)
    find_negatives(numerics)
    numerics.reduce(:+)
  end

  def find_negatives(numerics)
    negatives = numerics.select { |n| n < 0 }
    fail "negatives not allowed: #{negatives.join(', ')}" if negatives.any?
  end

  def supports?
    valid_delimiters.include?(@numbers[2, 1])
  end

  def delimiter
    return ',' unless @numbers.match(%r{^//})
    supports? ? @numbers[2, 1] : fail('Unsupported delimiter')
  end
end
\$\endgroup\$
5
  • \$\begingroup\$ so, delimiter is set only on the first add (or in the constructor)? \$\endgroup\$ – Uri Agassi Feb 23 '14 at 17:48
  • \$\begingroup\$ It's set in add, the constructor only creates the string I want to evaluate. @UriAgassi \$\endgroup\$ – user27606 Feb 23 '14 at 17:52
  • \$\begingroup\$ But it is set only on the first time you call add, right? \$\endgroup\$ – Uri Agassi Feb 23 '14 at 18:02
  • \$\begingroup\$ Could you add the requirements for the class (what happens when a string ends in \n, etc...) \$\endgroup\$ – Uri Agassi Feb 23 '14 at 18:03
  • \$\begingroup\$ I have tests for this, but here's the specs for the code: osherove.com/tdd-kata-1 @UriAgassi \$\endgroup\$ – user27606 Feb 23 '14 at 18:04
1
\$\begingroup\$

Some of the code seems to be missing - I can't see where you assign @numbers. I'm going to assume that it is set to "//+\n1+2" or the like.

DRY - twice in your code you calculate the delimiter (@numbers[2,1]), either pass it as an argument, or make it a method.

Btw, you seem to have a syntax error there, you might have meant to say supports? ? @numbers... instead.

Be clear, not clever you show good control in ranges, array, chars, etc., but still the following will be much clearer to future readers of your code (you included!)

ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"

(if you don't like to see escaped sequences, you can use %{!"\#$%&'()*+,-.:;<=>?@} instead)

Be Ruby - when you need to return a boolean, which is returned on a condition, there is no need to explicitly say .. ? true : false, simply return the condition result

Harness the power of regular expressions - when matching strings, captures are set to $n variables - you can use this to check your delimiter more elegantly:

ValidDelimiters = "!\"\\#$%&'()*+,-.:;<=>?@"

def supports?(delim)
   ValidDelimiters.include? delim
end

def delimiter
  return ',' unless @numbers.match(%r{^//(.)})
  delim = $1
  supports?(delim) ? delim  : fail('Unsupported delimiter')
end

Edit - with the new code

Naming - choose names that convey the meaning of the member - @numbers implicitly say it is a group of numbers, although it is actually a string... better name might be input_expression or something like that.

Redundant code with side-effects - your constructor accepts a string as input, which you assign to @numbers. This is not part of the requirements, and you probably don't have tests for that, but it may have serious impact on your result!

Also, for each time you call add, the input is concatenated to the end of the previous result (also not part of the requirements), which will break your code.

Assume nothing - Ruby's to_i method is very liberal - anything that is not a number will be parsed to - 0, so an input like "1,2,goat,3" will give a valid result of 6...

When state is redundant - when assuming each add is supposed to be a separate calculation - no state is needed. You can simply pass the expression around to your helper methods, and return the result. You can even turn your class into a module, with all methods being class methods, and the usage will be `StringCalculator.add('//+\n1+2')

\$\endgroup\$
8
  • \$\begingroup\$ I'll update this with how I append numbers. Sorry about that. Give me just a second, don't go anywhere! \$\endgroup\$ – user27606 Feb 23 '14 at 17:29
  • \$\begingroup\$ I thought about making this procedural after realizing that when my tests broke after calling add twice. So I'm assuming a module would just include methods without being object-oriented (which is what you're saying about the object state.) What exactly are you saying about to_i. Did I do something wrong? \$\endgroup\$ – user27606 Feb 23 '14 at 18:52
  • \$\begingroup\$ What would your code return if I called calc.add('1,2,goat,3')? Is this what you expect it to return? \$\endgroup\$ – Uri Agassi Feb 23 '14 at 18:54
  • \$\begingroup\$ It returned six. So it did in fact parse goat into zero. By converting this into a module, this would be all procedural, correct? \$\endgroup\$ – user27606 Feb 23 '14 at 18:58
  • 1
    \$\begingroup\$ Since all your methods should be class methods (def self.add(expression), you don't need to include the module, simply call the method is your test (expect(StringCalculator.add(...)).to ...) \$\endgroup\$ – Uri Agassi Feb 24 '14 at 4:43

Your Answer

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