Refactoring a Delimiter 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 &lt;&lt; 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(&amp;:chr).join
  end

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

  def find_negatives(numerics)
    negatives = numerics.select { |n| n &lt; 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
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 = "!\"\\#$%&amp;'()*+,-.:;&lt;=&gt;?@"


(if you don't like to see escaped sequences, you can use %{!"\#$%&amp;'()*+,-.:;&lt;=&gt;?@} 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 = "!\"\\#$%&amp;'()*+,-.:;&lt;=&gt;?@"

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