# Refactoring a String Calculator

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


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

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

• 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!
– user27606
Mar 1, 2014 at 23:37