Refactoring a Delimiter

Question

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

• Uri Agassi: so, delimiter is set only on the first add (or in the constructor)?
• user27606: It's set in add, the constructor only creates the string I want to evaluate. @UriAgassi
• Uri Agassi: But it is set only on the first time you call add, right?
• Uri Agassi: Could you add the requirements for the class (what happens when a string ends in \n, etc...)
• user27606: I have tests for this, but here's the specs for the code: osherove.com/tdd-kata-1 @UriAgassi

Answer 1

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')

• user27606: I'll update this with how I append numbers. Sorry about that. Give me just a second, don't go anywhere!
• user27606: 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?
• Uri Agassi: What would your code return if I called calc.add('1,2,goat,3')? Is this what you expect it to return?
• user27606: It returned six. So it did in fact parse goat into zero. By converting this into a module, this would be all procedural, correct?
• Uri Agassi: 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 ...)