1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ What is *args for? Seems irrelevant. Are these classes ActiveRecord models, or something else? What about LocationLessonService — is it your own code, or a third-party remote API? It's hard to advise you how to improve the code without knowing contextual information like that. \$\endgroup\$ – 200_success Jul 1 '17 at 6:54
  • \$\begingroup\$ This is a sidekiq worker. github.com/mperham/sidekiq \$\endgroup\$ – Bruno Castro Jul 1 '17 at 23:18
  • \$\begingroup\$ All classes are ActiveRecord models and this service is my own piece of code to consume the data of timetables \$\endgroup\$ – Bruno Castro Jul 1 '17 at 23:20
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ This is the data structure that the programmers who created the project used. Each location has its own timetables. \$\endgroup\$ – Bruno Castro Jul 1 '17 at 23:17
0
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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