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?

• tadman: In situations like this you really want to use arrays of objects you call methods on and things like Dir.glob to auto-load in everything in a directory. What does one of these add_numerals methods look like?
• 200_success: The Single Responsibility Principle is probably irrelevant to improving this code. What do the various adders in the included files look like? What is this code supposed to accomplish? Can you give an example of how it is to be used? Explain the purpose of the code, and make that the title of the question. See How to Ask.

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('')