1

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!

Improve this question
1
  • Looks good to me on a first glance, can you give some example data that gets ordered incorrectly? Commented Dec 5, 2023 at 5:37

1 Answer 1

Reset to default
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)
  1. 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

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Thanks so much for this! I appreciate the refactoring, but I think I have another error within this code. I'm trying to find Stores that match any of the Types and any of the locations, instead of needing to match all of them. Is there an easy refactoring to cover that use case? Thanks again!
I dont think you have a code problem but you have a database design problem. I can't see a way to chain that many or conditions in idiomatic way.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.