user181452 user181452 - 6 months ago 26
Ruby Question

Refactoring nested loops in a Ruby method that exports to CSV

I'm new to Ruby, and I have to export information to CSV.
I wrote this code and I don't really like it. I don't know how I can refactor it and get rid of the nested loops.

My relations are the following: Order has many Moves, Move has many Stops.

I have to export all of this to CSV, so I will have multiple lines for the same order!

def to_csv
CSV.generate(headers: true) do |csv|
csv << h.t(self.first.exported_attributes.values.flatten) # headers
self.each do |order|
order.moves.map do |move|
move.stops.map do |stop|
order_data = order.exported_attributes[:order].map do |attributes|
order.public_send(attributes)
end
move_data = order.exported_attributes[:move].map do |attributes|
move.decorate.public_send(attributes)
end
stop_data = order.exported_attributes[:stop].map do |attributes|
stop.decorate.public_send(attributes)
end
csv << order_data + move_data + stop_data
end
end
end
end
end


It's not good quality code ..

I did this yesterday:

def to_csv
CSV.generate(headers: true) do |csv|
csv << h.t(self.first.exported_attributes.values.flatten) # headers
self.each do |order|
order.moves.each do |move|
move.stops.each do |stop|
csv << order.exported_attributes[:order].map { |attr| order.public_send(attr) } +
order.exported_attributes[:move].map { |attr| move.decorate.send(attr) } +
order.exported_attributes[:stop].map { |attr| stop.decorate.send(attr) }
end
end
end
end
end


Update:

Thanks for the answer below, still can't get rid of the nested loops but at least it's structured well without key value big array :)

def to_csv
CSV.generate(headers: true) do |csv|
csv << h.t(Order.first.decorate.exported_attributes + Move.first.decorate.exported_attributes +
Stop.first.decorate.exported_attributes)
self.each do |order|
order.moves.each do |move|
move.stops.each do |stop|
csv << order.exported_values + move.decorate.exported_values + stop.decorate.exported_values
end
end
end
end
end


with these in the abstract decorator class:

def exported_attributes
[]
end

def exported_values
exported_attributes.map { |attr| self.public_send(attr) }
end

Answer

The biggest smell I smell isn't the nested loops, but the near-duplication of how the values are gotten from each model.

Let's extract that duplication into similar methods with the same name, exported_values, on Order, Move and Stop:

class Order
  def exported_values
    exported_attributes[:order].map { |attrs| { public_send(attrs) }
  end
end

class Move
  def exported_values
    order.exported_attributes[:stop].map { |attrs| { decorate.public_send(attrs) }
  end
end

class Stop
  def exported_values
    move.order.exported_attributes[:move].map { |attrs| { decorate.public_send(attrs) }
  end
end

and use them in to_csv:

def to_csv
  CSV.generate(headers: true) do |csv|
    csv << h.t(first.exported_attributes.values.flatten) # headers
    each do |order|
      order_values = order.exported_values
      order.moves.each do |move|
        order_and_move_values = order_values + move.exported_values
        move.stops.each do |stop|
          csv << order_and_move_values + stop.exported_values
        end
      end
    end
  end
end

The above has some additional minor improvements:

  • Get and concatenate the exported values in the outermost possible loops for efficiency.
  • Loop over moves and stops with each rather than with map, since the loops are done for side effects rather than return values.
  • Remove unnecessary uses of self..

Now to_csv isn't so bad.

Next we could try to refactor the exported_values methods into a single method.

  • Perhaps Order#exported_attributes could be broken up into a method on each class that takes no arguments and returns only that class's exported attributes.

  • The other difference between the methods is that Order doesn't need .decorator but the other classes do. If it has a decorator, just use that instead of the actual order; if not, just give it a fake one:

    class Order
      def decorator
        self
      end
    end
    

You could then define a single exported_values method in an module and include it in all three classes:

def exported_values
  exported_attributes.map { |attrs| { decorator.public_send(attrs) }
end

Possibly exported_attributes and exported_values should be defined on the decorators rather than on the models; I'm not sure whether your decorators are for CSV generation or only for other purposes.

Comments