2
\$\begingroup\$

I've refactored the string calculator kata as much as I could and tried to maintain single responsibility. Is there anything I could factor differently? The specs can be reviewed here.

module StringCalculator
  def self.add(string)
    string.end_with?("\n") ? fail('ends in newline') : solve(string)
  end

  def self.solve(string)
    verify(string)
    custom = delimiter(string)
    numerics = string.gsub(/\n/, custom).split(custom).map(&:to_i)
    numerics.reject { |n| n > 1000 }.reduce(0, :+)
  end

  def self.delimiter(string)
    string[0, 2] == '//' ? string[2, 1] : ','
  end

  def self.verify(string)
    find = string.scan(/-\d+/)
    fail("negatives not allowed: #{find.join(', ')}") if find.any?
  end

  private_class_method :verify, :delimiter, :solve
end

More importantly, I've thought of a case that involves the delimiter of a string that begins with // to be a number (which is considered invalid). I've made a quick way of solving that issue like so. Any suggestions on that?

Invalid case:

def self.delimiter(string)
  string[0, 2] == '//' ? check(string) || string[2, 1] : ','
end

def self.check(string)
  fail("invalid delimiter: #{string[2, 1]}") if string[2, 1] =~ /[0-9]/
end
\$\endgroup\$
1
\$\begingroup\$

I think your code is pretty good. The main opportunity for improved clarity I saw was to consolidate your validation code into one place. In your code, validations are sprinkled across various methods, so that you are mixing the responsibility of "validation" and "work".

Here's a refactoring (not tested) which puts all the validation in one place, and gets it out of the way up front. That way, the actual "work" you are doing becomes simpler to express, because you know your inputs are clean and meet all your assumptions at that point.

Also, note that you don't need to specify 0 as the first argument of reduce.

module StringCalculator

  def self.add(string)
    verify(string)
    delim = delimiter(string)
    numerics = string.gsub(/\n/, delim).split(delim).map(&:to_i)
    numerics.reject { |n| n > 1000 }.reduce(:+)
  end

  #######################
  ## Verification Code
  #######################

  def self.verify(string)
    verify_doesnt_end_with_newline(string)
    verify_no_negatives(string)
    verify_custom_delim(string) if has_custom_delim?(string)
  end


  def self.verify_doesnt_end_with_newline(string)
    fail('ends in newline') if string.end_with?("\n")
  end

  def self.verify_no_negatives(string)
    find = string.scan(/-\d+/)
    fail("negatives not allowed: #{find.join(', ')}") if find.any?
  end

  def self.verify_custom_delim(string)
    custom_delim = get_custom_delim(string)
    fail("invalid delimiter: #{custom_delim}") if custom_delim =~ /[0-9]/
  end

  #######################
  ## Delimiter Utilities
  #######################

  def self.delimiter(string)
    return ',' unless has_custom_delim?(string)
    get_custom_delim(string)
  end

  def self.has_custom_delim?(string)
    string[0, 2] == '//'
  end

  def self.get_custom_delim(string)
    string[2, 1]
  end

  private_class_method :verify, :verify_doesnt_end_with_newline, :verify_no_negatives, :verify_custom_delim, :delimiter, :has_custom_delim?, :get_custom_delim
end
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the feedback! The reason I have 0 as an initial value is because the spec says to return 0 for an empty string. Originally, I checked to see if the string was empty, with a early return. Then I realized that I didn't need to check, I could return 0 if there were no numbers to add within reduce. Good points on the validations as well. I'll consider these ideas! \$\endgroup\$ – user27606 Mar 1 '14 at 23:37

Your Answer

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