Weekly job to create or update the lessons in all locations

Question

I'm working in a project with the following structure.

A location can have many timetables with many lessons.

I need to perform a job in all locations every sunday to create or update the lessons.

def perform(*args)
    locations = Location.all
    locations.each do |location|
      timetables = LocationLessonService.fetch(location.id)["timetables"]
      timetables.each do |timetable|
        lessons = timetable["lessons"]
        lessons.each do |lesson|
          Lesson.find_or_initialize_by(external_id: lesson['external_id'])
            .update_attributes(period.merge(location_id: location.id))
        end
      end  
    end
  end

Is it possible to remove this nested iteration?Is it possible to optimize this function using the gem virtus or something else?

Answer 1

The code as written is fine. And I say that as hater of all things nested. One way or the other, you have nested loops. One option is to retain that structure explictly, as it currently is, but to clean it up by removing temporary named variables that add clutter but not clarity:

def perform(*args)
  Location.all.each do |location|
    timetables = LocationLessonService.fetch(location.id)["timetables"]
    timetables.each do |timetable|
      timetable["lessons"].each do |lesson|
        Lesson.find_or_initialize_by(external_id: lesson['external_id'])
          .update_attributes(period.merge(location_id: location.id))
      end
    end  
  end
end

What makes it difficult to tease these loops apart into named methods is that the innermost loop references the location.id from the outermost loop.

That said, I'm wondering why the location isn't simply a property of the timetable? Or can the same timetable exist in multiple locations? Because if the location were a property of the timetable, you wouldn't have the problem I mentioned in the last paragraph.

That is to say, while I don't consider the code itself problematic, there may be better ways to organize your data. But it's hard to say without knowing more.

Answer 2

One way to do it is to break out the nested loops using map to create snapshots of the array at different points in the transformation and then reference those using with_index.

  def perform(*args)
    locations = Location.all
    timetables = locations.map do |location|
      LocationLessonService.fetch(location.id)["timetables"]
    end
    lessons = timetables.map { |timetable| timetable["lessons"] }
    lessons.each_with_index do |lesson, index|
       location = locations[index]
       Lesson.find_or_initialize_by(external_id: lesson['external_id'])
             .update_attributes(period.merge(location_id: location.id))
    end
  end