Many very similar functions, spaghetti code fix?

Question

I have approx 11 functions that look like this:

def pending_acceptance(order_fulfillments)
  order_fulfillments.each do |order_fulfillment|
    next unless order_fulfillment.fulfillment_time_calculator.
        pending_acceptance?; collect_fulfillments(
          order_fulfillment.status,
          order_fulfillment
        )
  end
end

def pending_start(order_fulfillments)
  order_fulfillments.each do |order_fulfillment|
    next unless order_fulfillment.fulfillment_time_calculator.
        pending_start?; collect_fulfillments(
          order_fulfillment.status,
          order_fulfillment
        )
  end
end

The iteration is always the same, but next unless conditions are different. In case you wonder: it's next unless and ; in it because RuboCop was complaining about it. Is there a solution to implement it better? I hate this spaghetti code. Something like passing the condition into "iterate_it" function or so...

edit: Cannot just pass another parameter because the conditions are double sometimes:

def picked_up(order_fulfillments)
      order_fulfillments.each do |order_fulfillment|
        next unless
          order_fulfillment.handed_over_late? && order_fulfillment.
              fulfillment_time_calculator.pending_handover?
                collect_fulfillments(
                  order_fulfillment.status,
                  order_fulfillment
                )
      end
    end

edit2: One question yet: how could I slice a symbol, to get a user role from a status? Something like: :deliverer_started => :deliverer or 'deliverer'?

Answer 1

While next is handy it comes late(r) in the code and is thus a bit more difficult to grasp. I would first select on the list, then do the action. (Note that this is only possible if your 'check' does not have side effects like in order_fullfillment.send_email_and_return_false_if_fails).

So if tests can be complex I would start the refactoring by expressing the selection criteria and then pulling out the processing of these items (wich also matches more the method names you have given), somewhere in the middle it might look like this:

def pending_acceptance(order_fulfillments)
  order_fulfillments.select do |o|
    o.fulfillment_time_calculator.pending_acceptance?
  end
end

def picked_up(order_fulfillments)
  order_fulfillments.select do |order_fulfillment|
    order_fulfillment.handed_over_late? && order_fulfillment.
          fulfillment_time_calculator.pending_handover?
  end
end

def calling_code
  # order_fulfillments = OrderFulFillments.get_from_somewhere
  # Now, filter
  collect_fulfillments(pending_start      order_fulfillments)
  collect_fulfillments(picked_up          order_fulfillments)
end

def collect_fullfillments order_fulfillments
  order_fulfillments.each {|of| collect_fullfillment(of) }
end

You'll still have 11 (+1) methods, but imho you express more what you are up to - and your colleagues will grok what happens fast, too. Given your example and question I think you should aim for a simple, expressive solution. If you are more "hardcore", use the more functional lambda approach given in the other solutions. Also, note that these approaches could be combined (by passing an iterator).

Answer 2

You could use something like method_missing.

At the bottom of your class, put something like this:

def order_fulfillment_check(method, order_fulfillment)
  case method
    when "picked_up" then return order_fulfillment.handed_over_late? && order_fulfillment.fulfillment_time_calculator.pending_handover?
    ...
    ... [more case statements] ...
    ...
    else return order_fulfillment.fulfillment_time_calculator.send(method + "?")
  end
end

def method_missing(method_name, args*, &block)
  args[0].each do |order_fulfillment|
    next unless order_fulfillment_check(method_name, order_fulfillment); 
    collect_fulfillments(
        order_fulfillment.status,
        order_fulfillment
      )
  end
end

Depending on your requirements, you could check if the method_name starts with "pending_".

Please note, this code is untested, but it should be somewhere along the line.

Also, as a sidenote, order_fulfillment.fulfillment_time_calculator.some_random_method is actually a violation of the law of demeter. You might want to adress this.

Answer 3

You can pass another parameter when you use that parameter to decide what condition to check. Just store all possible conditions as lambdas in a hash:

FULFILLMENT_ACTIONS = {
  pending_acceptance: lambda { |fulfillment| fulfillment.fulfillment_time_calculator.pending_acceptance? }, 
  pending_start:      lambda { |fulfillment| fulfillment.fulfillment_time_calculator.pending_acceptance? },
  picked_up:          lambda { |fulfillment| fulfillment.handed_over_late? && fulfillment.fulfillment_time_calculator.pending_handover? }
}

def process_fulfillments(type, order_fulfillments)
  condition = FULFILLMENT_ACTIONS.fetch(type)

  order_fulfillments.each do |order_fulfillment|
    next unless condition.call(order_fulfillment)
    collect_fulfillments(order_fulfillment.status, order_fulfillment)
  end
end

To be called like:

process_fulfillments(:pending_acceptance, order_fulfillments)
process_fulfillments(:pending_start, order_fulfillments)
process_fulfillments(:picked_up, order_fulfillments)

Answer 4

you can make array of strings

arr = ['acceptance','start', ...]

in next step:

arr.each do |method|

  define_method ( 'pending_#{method}'.to_sym ) do |order_fulfillments|
    order_fulfillments.each do |order_fulfillment|
      next unless order_fulfillment.fulfillment_time_calculator.
      send('pending_#{method}?'); collect_fulfillments(
            order_fulfillment.status,
            order_fulfillment
          )
    end
  end
end

for more information about define_method