# Use SRP to refactor this Ruby Class

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?

## migrated from stackoverflow.comSep 14 '16 at 19:40

This question came from our site for professional and enthusiast programmers.

• 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? – tadman Sep 14 '16 at 19:36
• 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. – 200_success Sep 16 '16 at 14:01

## 1 Answer

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