skip to Main Content

I’ve got a method like this:

def transfers(material, capacity, category, created_at)
  transfers_range = Transfer.first.created_at..Transfer.last.created_at if created_at.nil?
  Transfer.where(
    sender_dispensary_id: id,
    status: %i[dispatched released returned],
    capacity: capacity,
    material: material,
    created_at: created_at.nil? ? transfers_range : Time.given_month_range(created_at)
  ).or(
    Transfer.where(
      receiver_dispensary_id: id,
      status: %i[dispatched released returned],
      capacity: capacity,
      material_id: material,
      created_at: created_at.nil? ? transfers_range : Time.given_month_range(created_at)
    )
  )
end

It’s working, but is there a way to avoid query for transfer_range? I mean… If created_at == nil, then this function should skip created_at column, like it was not included in query

3

Answers


  1. Yes, the transfers_range query can be avoided by breaking your query into multiple lines and adding a condition on created_at presence separately. You can also combine both the OR queries into a single query like this:

    def transfers(material, capacity, category, created_at)
      transfer_data = Transfer.where('sender_dispensary_id = ? OR receiver_dispensary_id = ?', id).where(status: %i[dispatched released returned], capacity: capacity, material_id: material)
      transfer_data = transfer_data.where(created_at: Time.given_month_range(created_at)) if created_at.present?
      transfer_data
    end
    
    Login or Signup to reply.
  2. When created_at is nil then transfers_range basically covers all transfers and therefore the condition is pointless and I simple would query by created_at in that case.

    def transfers(material, capacity, category, created_at)
      transfers = Transfer
        .where(status: %i[dispatched released returned], capacity: capacity, material_id: material)
        .where('sender_dispensary_id = :id OR receiver_dispensary_id = :id', id: id)
    
      transfer = transfer.where(created_at: Time.given_month_range(created_at)) if created_at
    
      transfer
    end
    
    Login or Signup to reply.
  3. You can consolidate the created_at query logic so it’s in one place and you don’t have to repeat it.

      created_at = if created_at.nil?
        Transfer.first.created_at..Transfer.last.created_at
      else
        Time.given_month_range(created_at)
      end
    

    You also don’t need to repeat the whole where condition twice. You want the equivalent of…

    where status in ('dispatched', 'released', 'returned')
      and capacity = ?
      and material = ?
      and created_at = ?
      and (
        sender_dispensary_id = ?
        or
        receiver_dispensary_id = ?
      )
    

    You do that like so:

      Transfer
        .where(
          status: %i[dispatched released returned],
          capacity: capacity,
          material: material,
          created_at: created_at
        )
        .where(
          Transfer
            .where(receiver_dispensary_id: id)
              .or(Transfer.where(sender_dispensary_id: id))
        )
      )
    

    You can make this even more concise, and hide the details, by putting the logic into a scopes.

    class Transfer < Application
      scope :by_dispensary_id ->(id) {
        where(receiver_dispensary_id: id)
          .or(where(sender_dispensary_id: id))
      }
    
      scope :by_created_month ->(time) {
        if time.nil?
          where(created_at: first.created_at..last.created_at)
        else
          where(created_at: Time.given_month_range(time))
        end
      }
    end
    

    And now the query is much simpler.

      Transfer
        .by_dispensary_id(id)
        .by_created_month(created_at)
        .where(
          status: %i[dispatched released returned],
          capacity: capacity,
          material: material
        )
      )
    
    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search