2
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ 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? \$\endgroup\$
    – tadman
    Sep 14, 2016 at 19:36
  • \$\begingroup\$ 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. \$\endgroup\$ Sep 16, 2016 at 14:01

1 Answer 1

4
\$\begingroup\$

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('')
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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