3
\$\begingroup\$

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
\$\endgroup\$
3
\$\begingroup\$

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
    
\$\endgroup\$
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.