How can do a performance friendly version of this Ruby code?

Question

Hey guys i'm new to RoR and ruby in general.

In this project i am working at the moment i'm showing 4 images of a vehicle that were previously uploaded by users.

In my class 'Vehicle' i have a singleton method - that returns an array with urls for attached images of a vehicle object - which i call like this Vehicle.vehicle_images(@vehicle.id). I'd like to make this method look more performatic and less hard-coded.

my vehicle class:

class Vehicle < ApplicationRecord
  has_many :vehicles_attachment, dependent: :destroy
  has_many :vehicles_attachment_imagem, -> { where({ type_attachment: 'imagem' }) }, class_name: 'VehiclesAttachment', dependent: :destroy

   def self.vehicle_images(vehicle_id)
     if vehicle_id
       images = VehiclesAttachment.where({ vehicle_id: vehicle_id, type_attachment: :imagem })
       urls = []
       if !images.nil? && !images.empty?
         count = 0
         images.each do |img|
           urls << if img.attachment.file?
                     "https:#{img.attachment.url(:regular)}"
                   else
                     4.times do
urls << ActionController::Base.helpers.image_url('viatura.jpg', digest: false).to_s
                     end
                   end
          count += 1
         end
         if count < 4
           (4 - count).times do
             urls << ActionController::Base.helpers.image_url('viatura.jpg', digest: false).to_s
           end
         end
       else
         4.times do
           urls << ActionController::Base.helpers.image_url('viatura.jpg', digest: false).to_s
         end
       end
     else
       4.times do
         urls << ActionController::Base.helpers.image_url('viatura.jpg', digest: false).to_s
       end
     end
     urls
   end
end

Can you help me?

Answer 1

As a first step it could be simplified to this:

def self.vehicle_images(vehicle_id)
  blank_image_url = ActionController::Base.helpers.image_url('viatura.jpg', digest: false).to_s

  urls = []

  if vehicle_id
    VehiclesAttachment.where(vehicle_id: vehicle_id, type_attachment: :imagem).each do |image|
      urls << img.attachment.file? ? "https:#{img.attachment.url(:regular)}" : blank_image_url
    end
  end

  urls << blank_image_url while urls.size < 4

  urls
end

But this is just a first step. The next steps could be:

• Determine if you have to deal with blank vehicle_id. If not then an instance method on a vehicle would be better than a class method. And you would be able to use the defined association instead of the where on the class.
• I would investigate if it is really possible that you sometimes get only some images returned and really have to fill in the dummy image between them. Or if it would be enough to only fill in dummy images at the end if there are less than 4. That would simplify the method further.
• I wonder if you sometimes have to deal with more than 4 images? Your code currently doesn't handle that.
• Perhaps you could add variations to ensure that there are always exactly the four images?