# String calculator kata in Ruby

I've completed the string calculator kata in Ruby (spec from here - http://osherove.com/tdd-kata-1/). All tests currently pass.

Could it be refactored further to improve readability? And could I make my testing better?

string_calculator.rb

class StringCalculator

def initialize
end

raise 'only accepts a string' unless string_of_numbers.is_a?(String)
string_array = string_of_numbers.split(/[^0-9-]+/)
integer_array = string_array.map(&:to_i)
raise "cannot accept negatives - #{check_for_negatives(integer_array)}" if
check_for_negatives(integer_array)
integer_array.inject(0){|sum,x| x <= 1000? sum + x : sum }
end

def check_for_negatives(integer_array)
negatives_array = integer_array.select{ |i| i<0 }
if negatives_array.length > 0
return negatives_string = negatives_array.join(",")
else
return false
end
end

end


string_calculator_spec.rb

require 'string_calculator'

describe StringCalculator do

subject(:calculator) { described_class.new }

it 'should accept a string' do
end

it 'should not accept other data types' do
expect{ calculator.int_add(123) }.to raise_error('only accepts a string')
expect{ calculator.int_add(['123']) }.to raise_error('only accepts a
string')
end

it 'should return 0 for an empty string' do
end

it 'should return a number if the passed string contains no delimiters' do
end

it 'should return the sum of the numbers in the passed string, if the passed
string contains comma delimiters' do
end

it 'should return the sum of the numbers in the passed string, if the passed
string contains new line delimiters' do
end

it 'should handle multiple random delimiters' do
end

it 'should not accept negative numbers' do
negatives - -2")
end

it 'should ignore numbers larger than 1000' do
end

end


The changes I will propose are from ruby-style-guide since that is accepted by the majority of Ruby developers.

What you can improve:

• Remove initialize method since there is no need for it in your case.
• Use white space to separate groups of logic, something along these lines:

raise 'only accepts a string' unless string_of_numbers.is_a?(String)

string_array = string_of_numbers.split(/[^0-9-]+/)   integer_array = string_array.map(&:to_i)

raise "cannot accept negatives -#{check_for_negatives(integer_array)}" if  check_for_negatives(integer_array)

integer_array.inject(0){|sum,x| x <= 1000? sum + x : sum }

• It's not a good practice to hard code the data in the name, instead of string_of_numbers replace with numbers, and instead of integer_array you can figure something else (naming is hard).

• Use public and private API methods, since int_add is the only method your class is using, you can rename it to add, and make check_for_negatives private so the reader will know to rely only on add as a stable method.

• Ruby returns the last expression, so you don't need an assignment for return negatives_string = negatives_array.join(","), it can be negatives_array.join(",")

• You can reduce the check_for_negatives method to this:

def check_for_negatives(numbers)
negatives = numbers.select{ |i| i < 0 }
# Ruby return the last statement which is the negatives string,
# or nil, that is treated as false
negatives.join(",") if negatives.length > 0
end


Try to have more meaningful names. For example, instead of string_of_numbers use input; instead of integer_array use something like numbers.

Also, split is intended to split on a delimiter. You're splitting on anything that's not a number, which is a little odd, almost like a double negative. Instead, consider using scan:

numbers = input.scan(/[0-9-]+/).map(&:to_i)


Note that you're calling check_for_negatives twice, and in the if statement you are discarding the value. If you did the if first, there'd be less repetition and it would be more self-explanatory.

if negatives = get_negatives(numbers)
raise "cannot accept negatives - #{negatives}"
end