Code organisation question

Bit of an odd one but please bear with me:

I have a number of models: Album and Photo.

I wished to be able to update tags on photos from Flickr by clicking a button in my UI.

First I started by creating a rake task that loops through all Photos, grabbed the tags from Flickr and updated the Photo.

I then moved the logic to update a single Photo into a method on the Photo model itself, I called it update_tags_from_flickr.

I then created another new class method on Photo called update_tags_from_flickr but this one does the same as my rake task and loops through ALL the photos. That method calls the instance method for each Photo. All Good.

Right, I then added a button next to the name of each Album in my admin area that calls a controller action and passes in the Album id. Like so:

def update_album_tags
    @album = Album.friendly.find(params[:album_id])

    @album.update_tags_from_flickr
end

I added a new instance method to Album that iterates its photos and calls update_tags_from_flickr on each one. All good here.

I then thought it might be nice if each “photo tags” update was done as a background job. I created a UpdateTagsFromFlickrJob and now here is where my confusion started:

The job accepts a Photo. but I had to modify my controller action to add a loop through the Album photos and call the background job on each. Like so:

def update_album_tags
    @album = Album.friendly.find(params[:album_id])

    @album.photos.each do |photo|
        UpdateTagsFromFlickrJob.perform_later(photo)
    end
end

I notice that I have that Album.photos loop in two places. Here in the controller and once (from earlier) in my Album model.

Is this correct?

I know I could update the one in the Album model to use the background job, but not sure if I should? Maybe I want a version that doesn’t use a job? I’m unsure.

Is there a way to reduce the duplication of that loop?

What would you suggest?

Thanks, Neil

1 Like

Hi, It was a bit complicated following your explanation without the actual code.

Given what I understood, would something like this work ?

class Photo
  def update_tags_from_flickr
    # connect to flickr
  end

  def self.update_tags_from_flickr_later(scope = all)
    scope.find_each do |photo| # or find_in_batches
      photo.update_tags_from_flickr_later
    end
  end

  def update_tags_from_flickr_later
    UpdateTagsFromFlickrJob.perform_later(self)
  end
end

class Album
  has_many :photos do
    def update_tags_from_flickr_later
      self.update_tags_from_flickr_later(self.scope)
    end
  end

  def update_tags_from_flickr_later
    photos.update_tags_from_flickr_later
  end
end

class UpdateTagsFromFlickrJob
  def perform(photo) # or photo_id
    photo.update_tags_from_flickr
  end
end

class SomewhereInTheAdmin
  def update_album_tags
    @album = Album.friendly.find(params[:album_id])
    @album.update_tags_from_flickr_later
  end
end