String calculator kata in Ruby

Question

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
 def int_add(string_of_numbers)
 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
 expect{ calculator.int_add('1,2,3') }.not_to raise_error
 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
 expect(calculator.int_add('')).to eq(0)
 end
 it 'should return a number if the passed string contains no delimiters' do
 expect(calculator.int_add('123')).to eq (123)
 end
 it 'should return the sum of the numbers in the passed string, if the passed 
string contains comma delimiters' do
 expect(calculator.int_add('12,34')).to eq(46)
 end
 it 'should return the sum of the numbers in the passed string, if the passed 
string contains new line delimiters' do
 expect(calculator.int_add("12\n34\n56")).to eq(102)
 end
 it 'should handle multiple random delimiters' do
 expect(calculator.int_add("//;\n1;2")).to eq(3)
 end
 it 'should not accept negative numbers' do
 expect{ calculator.int_add("123,-2") }.to raise_error("cannot accept 
negatives - -2")
 end
 it 'should ignore numbers larger than 1000' do
 expect(calculator.int_add("//;\n1;2:1001")).to eq(3)
 end
end

Answer 1

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
  

Answer 2

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