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

Theo Chirica · Accepted Answer · 2017-08-17 13:45:05Z

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

Mark Thomas · Accepted Answer · 2017-08-20 01:23:39Z

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

Stack Exchange Network

String calculator kata in Ruby

2 Answers 2

You must log in to answer this question.

Hot Network Questions

String calculator kata in Ruby

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions