Use SRP to refactor this Ruby Class

Question

I have a class, and it does one thing, It adds numerals to the numerals array.

In doing so it uses a number of private methods and has ended up looking a little bit sickening...

brace yourself...

require './lib/number_interpreter'
require './lib/numeral_adders/m_adder'
require './lib/numeral_adders/cm_adder'
require './lib/numeral_adders/cd_adder'
require './lib/numeral_adders/d_adder'
require './lib/numeral_adders/c_adder'
require './lib/numeral_adders/xc_adder'
require './lib/numeral_adders/xl_adder'
require './lib/numeral_adders/l_adder'
require './lib/numeral_adders/x_adder'
require './lib/numeral_adders/ix_adder'
require './lib/numeral_adders/iv_adder'
require './lib/numeral_adders/v_adder'
require './lib/numeral_adders/i_adder'

# A class for adding individual numerals
class NumeralAdder
  attr_reader :num_to_convert,
              :num_interpreter,
              :numeral

  def initialize(num_to_convert)
    @num_to_convert = num_to_convert
    @num_interpreter = NumberInterpreter.new(num_to_convert)
    @numeral = []
  end

  def add_numerals
    @numeral << m_adder.add_numerals.split('')
    @numeral << cm_adder.add_numerals.split('')
    @numeral << cd_adder.add_numerals.split('')
    @numeral << d_adder.add_numerals.split('')
    @numeral << c_adder.add_numerals.split('')
    @numeral << xc_adder.add_numerals.split('')
    @numeral << xl_adder.add_numerals.split('')
    @numeral << l_adder.add_numerals.split('')
    @numeral << x_adder.add_numerals.split('')
    @numeral << ix_adder.add_numerals.split('')
    @numeral << iv_adder.add_numerals.split('')
    @numeral << v_adder.add_numerals.split('')
    @numeral << i_adder.add_numerals.split('')
    flatten_numeral
  end

  private

  def flatten_numeral
    @numeral = @numeral.flatten
  end

  def m_adder
    MAdder.new(@num_interpreter.thousands)
  end

  def cm_adder
    CMAdder.new(@num_interpreter.hundreds)
  end

  def cd_adder
    CDAdder.new(@num_interpreter.hundreds)
  end

  def d_adder
    DAdder.new(@num_interpreter.hundreds)
  end

  def c_adder
    CAdder.new(@num_interpreter.hundreds)
  end

  def xc_adder
    XCAdder.new(@num_interpreter.tens)
  end

  def xl_adder
    XLAdder.new(@num_interpreter.tens)
  end

  def l_adder
    LAdder.new(@num_interpreter.tens)
  end

  def x_adder
    XAdder.new(@num_interpreter.tens)
  end

  def ix_adder
    IXAdder.new(@num_interpreter.units)
  end

  def iv_adder
    IVAdder.new(@num_interpreter.units)
  end

  def v_adder
    VAdder.new(@num_interpreter.units)
  end

  def i_adder
    IAdder.new(@num_interpreter.units)
  end
end

I'm thinking I need to separate the private class creations into their own class, but i'm not sure how.

Any tips?

Answer 1

To start out with all of the

require './lib/numeral_adders/<lib>

should be moved to a separate file like/lib/number_interpreter

Moving on, looking at repetitive parts of your code. The add_numerals method has a lot of lines like this:

@numeral << <type>_adder.add_numerals.split('')

and you have methods like this:

def <type>_adder
  <type>Adder.new(@num_interpreter.<unit>)
end

You can make this DRY by enumerating over a list defined elsewhere. For example:

# in the lib/number_interpreter.rb file
class NumberInterpreter
  Adders = {
    MAdder => :thousands,
    CMAdder => :hundreds
    # ...  
  }
end

# in the main file (numeral_adder.rb)

def add_numerals
  NumberInterpreter::Adders.each do |klass, unit|
    @numeral << adder(klass, unit).add_numerals.split('')
  end
  flatten_numeral
end

def adder(klass, unit)
  klass.new(@num_interpreter.send(unit))
end

Important concepts here include setting a class as a hash key then initializing it using new later on. It also uses send for programmatic method invocation. You're writing a generic adder class to make all that repetition unnecessary - its arguments are a class to initialize and a unit to convert the results with.

By the way your flatten_numeral is so simple that it'd be better to not make it a separate method. The flatten would probably be unnecessary if you use concat instead of << (shovel):

@numeral.concat adder(klass, unit).add_numerals.split('')