Chaining .where method in Rails API

Question

I have a web app built with a Rails API to collect submissions. I have a form being submitted that sends an email lead to certain Stores that have matching attributes (location, size, etc). They are matched based off the following fields

• Location
• Capacity (min and max)
• Store Type (hotel, dining, unique)

I also have an attribute for the store to receive all leads, that I want to override any of the previous options, if set to true.

In the hard_worker.rb in my Rails API, I have it checking the following fields, in order to query which Stores to send the Leads to. I know the chaining of conditionals isn't correct, but cannot figure out how to reformat it to send correctly. Any help would be greatly appreciated, Thanks!

def build_filters_obj
        filters = []
        filters.push 'location_north' if @lead.location_north
        filters.push 'location_east' if @lead.location_east
        filters.push 'location_south' if @lead.location_south
        filters.push 'location_west' if @lead.location_west
        filters.push 'location_other' if @lead.location_other
        
        return filters
    end

    def perform(lead_id)
        @lead = Lead.find(lead_id)

        lead_email = ValidEmail2::Address.new(@lead.email)
        UserNotifierMailer.send_signup_email(@lead).deliver if lead_email.valid?

        @stores = Store.all
        @stores = @stores.where.not(email: [nil, ''])

        n = @lead.guests_total.to_i
        @stores = @stores.where("capacity_min <= ? AND capacity_max >= ?", n, n)
        
        @stores = @stores.where(:type_unique => true) if @lead.store_type_unique
        @stores = @stores.where(:type_dining => true) if @lead.store_type_dining
        @stores = @stores.where(:type_hotel => true) if @lead.store_type_hotel
        
        filters = build_filters_obj
        filters.each do |filter|
            @stores = @stores.where(filter.to_sym => true)
        end

        @stores = @stores.or(Store.where(:receive_all => true))

        @stores.each do |store|
            store_email = ValidEmail2::Address.new(store.email)
            UserNotifierMailer.send_lead_email(store, @lead).deliver if store_email.valid?
        end
    end

I apologize if my code is atrocious, Ruby is not where I usually work and I'm sure I'm committing some beginner errors!

Answer 1

Biggest code smell I've noticed is there are lots of if checks, and it makes harder to reason with your code. Try to create hashes and call methods to clean the hash instead.

I would also suggest you to pass this code through rubocop. It will make your code feel more idiomatic and less wrong.

1. build_filters_obj

filters.push 'location_north' if @lead.location_north
filters.push 'location_east' if @lead.location_east
filters.push 'location_south' if @lead.location_south
filters.push 'location_west' if @lead.location_west
filters.push 'location_other' if @lead.location_other

Maybe we can at least make it ready to use. In ruby idiomatic way to name this would be as a noun and directly use it. Notice that we are not creating strings and not converting them into symbols later.

def location_filters
  {
    location_north: @lead.location_north,
    location_east: @lead.location_east,
    location_south: @lead.location_south,
    location_west: @lead.location_west,
    location_other: @lead.location_other
  }.filter { |key, value| value.present? }
end

# later on you would use this like this

@stores = @stores.where(location_filters)

2. if checks at store_type* related methods

You can build a hash then remove falsy values

type_filters = {
  type_unique: @lead.store_type_unique
  type_dining: @lead.store_type_dining
  type_hotel: @lead.store_type_hotel
}.filter { |key, value| value.present? }

# later on we can directly pass it to where method

@stores = @stores.where(type_filters)

So overall this is how you can call the query instead

def perform(lead_id)
    @lead = Lead.find(lead_id)

    lead_email = ValidEmail2::Address.new(@lead.email)
    UserNotifierMailer.send_signup_email(@lead).deliver if lead_email.valid?

    capacity = @lead.guests_total.to_i

    type_filters = { # consider moving to a private method called `type_filters`
      type_unique: @lead.store_type_unique
      type_dining: @lead.store_type_dining
      type_hotel: @lead.store_type_hotel
    }.filter { |key, value| value.present? }
      
    @stores = Store.where.not(email: [nil, ''])
                   .where("capacity_min <= ? AND capacity_max >= ?", capacity, capacity)
                   .where(type_filters)
                   .where(location_filters)
                   .or(Store.where(receive_all: true))

    @stores.each do |store|
        store_email = ValidEmail2::Address.new(store.email)
        UserNotifierMailer.send_lead_email(store, @lead).deliver if store_email.valid?
    end
end

https://rubyapi.org/3.2/o/hash#method-i-filter