Clean up messy code that query's based on multiple options

Question

I'm using Rails, but the underlying question here applies more broadly. I have a report page on my web app that allows the user to specify what they're filtering on, and query the database based on those filters (MongoDB).

The data is based around hotels, the user must first select the regions of the hotels (state_one, state_two, state_three), then the statuses of the hotels (planning, under_construction, operational), then an optional criteria, price range (200, 300, 400). Users can select multiple of each of these options.

My way of doing this currently is to create an empty array, iterate through each region, and push the region into the array if the user selected that region. Then, I'm iterating through THAT array, and assessing the status of the hotels in those regions, if any hotel has the status the user has selected, then I'm adding that hotel to a new empty array. Then I do the same thing for price range.

This works, but the code is offensively messy, here's an example of the code:

def find_hotel
  hotels = find_all_hotels
  first_array = []
  hotels.each do |hotel|
    if params[:options][:region].include? 'state_one' and hotel.state == :one
      first_array.push(hotel)
    elsif params[:options][:region].include? 'state_two' and hotel.state == :two
      first_array.push(hotel)
    elsif params[:options][:region].include? 'state_three' and hotel.state == :three
      first_array.push(hotel)
    end
  end

  second_array = []
  first_array.each do |hotel|
    if params[:options][:region].include? 'planning' and hotel.status == :planning
      first_array.push(hotel)
    elsif params[:options][:region].include? 'under_construction' and hotel.status == :under_construction
      first_array.push(hotel)
    elsif params[:options][:region].include? 'operational' and hotel.status == :operational
      first_array.push(hotel)
    end
  end

  third_array = []
  second_array.each do |hotel|
    # More of the same here, this could go on forever
  end
end

What are some better ways of achieving this?

Why do you have `hotel_is_in_state_one` instead of something like `hotel_state` being `:one`? — tadman, Apr 10 '17 at 23:13
ActiveRecord's `where` method will translate an array to a SQL `WHERE / IN` statement. So you should be able to do something like `Hotel.where(state: params[:options][:region], region: params[:options][:region])`. Of course you will want to filter your params through a permitted_params mechanism to protect from attack. — moveson, Apr 10 '17 at 23:17
@tadman I've written that code for the sake of this example, it's not my actual code, just something that I thought would illustrate what I want it to do, my actual application is different but I can't use it here because it isn't all mine. — Justin, Apr 10 '17 at 23:36
The reason this code is verbose and repetitive is because of the structure of the data. If you can align things properly you can often do this with very little effort needed. When I see things like `first_array` and `state two` that's a sign you're using the *Pile of Variables* approach to thing when really what you need is structured data: An array, a hash, or some object that encapsulates it all as a container. — tadman, Apr 10 '17 at 23:41
Are those states mutually exclusive for each record or is it a case of where zero or more may apply? This also seems relevant to the second set. I'm not sure why `region` would have things like "operational" in it. This seems very disorganized and that's causing a lot of friction. — tadman, Apr 10 '17 at 23:43
@tadman Sorry I'm a bit confused, just to clarify, my Hotel model `has_one :region`, `has_many :statuses`, `has_one :price_range`. I've edited my question to reflect this. How would you suggest I go about incorporating structured data here? — Justin, Apr 11 '17 at 00:37
I think you meant `second_array.push(hotel)` on your second array. Is that right? I sent you an edit suggestion to reflect that. — Gerry, Apr 11 '17 at 20:49

Gerry · Accepted Answer · 2017-04-11T03:31:19.980

1

How about this:

STATES   = [:one, :two, :three]
STATUSES = [:planning, :under_construction, :operational]
PRICES   = [200, 300, 400]

def find_hotel
  region = params[:options][:region]

  first_array  = set_array(region, find_all_hotels, STATES, :state)
  second_array = set_array(region, first_array, STATUSES, :status)
  third_array  = set_array(region, second_array, PRICES, :price_range)
end

def set_array(region, array, options, attribute)
  array.each_with_object([]) do |element, result|
    options.each do |option|
      result << element if region.include?(option) && element[attribute] == option
    end
  end
end

UPDATE

Added attribute parameter to set_array in order to make the code work with your updated example.

edited Apr 11 '17 at 03:31

answered Apr 10 '17 at 23:55

Gerry

10,337
3
31
40

The general shape of this is good, but since those arrays never change they really should be constant values declared at the class level. `OPTIONS = %w[ option_1 option_2 option 3 ]` for example. – tadman Apr 11 '17 at 00:38
Hey @Gerry, sorry but I'm new to all this and don't know how `each_with_object` works. I'm reading up on it now, but perhaps you could explain it to me here quickly in leymans terms? Also, I was hoping for `find_hotel` to return an array or hash of hotels that match the search criteria, how would I extend on your example to get such a result? – Justin Apr 11 '17 at 01:00
@Justin `each_with_object` loops the array and returns an object which type is defined as a parameter. In this case it returns an `Array` (`each_with_object([])`) named `result`. Is the same result as creating an array first and using it inside a regular `each` loop (like your example). Check here for more information: http://stackoverflow.com/questions/19064209/how-is-each-with-object-supposed-to-work – Gerry Apr 11 '17 at 01:10
@tadman Yes, definetly a better option; i’ll add an update. Justin are you using this methods inside a class? – Gerry Apr 11 '17 at 01:18
@Gerry I actually just created that method for the purpose of this example, I have edited the question to more accurately reflect the actual code I'm using. I can't use the actual code I'm working with because it isn't all mine. I'm sorry if my edit changes the relevance of your answer. – Justin Apr 11 '17 at 01:34
@Justin if i understand correctly, the hotels that match the search criteria will be on the third array. All methods in ruby return the last statement or expression in its code, so in this case you will get `third_array` as a return value. – Gerry Apr 11 '17 at 02:11
Another trick is `[ :state, :status, :price_range ].map do |attribute|` and combine the three `_array` variables into a single array-of arrays. That set can also be a constant since it's used repeatedly. – tadman Apr 11 '17 at 03:31
@tadman I thought about that (well, a Hash instead of Array) but couldn't figure out how to call `set_array` with different `array` parameters. Any ideas? – Gerry Apr 11 '17 at 15:57
@Gerry That chunk I put in there gives you an `attribute` variable you can feed through. – tadman Apr 11 '17 at 18:04

score 0 · Answer 2 · answered Apr 11 '17 at 12:34

Since second_array is empty, whatever you get by iterating over it (perhaps third_array) would also be empty.

def find_hotel
  hotels = find_all_hotels

  first_array = hotels
  .select{|hotel| params[:options][:region].include?("state_#{hotel.state}")}

  first_array += first_array
  .select{|hotel| params[:options][:region].include?(hotel.status.to_s)}

  second_array = third_array = []

  ...
end

Clean up messy code that query's based on multiple options

2 Answers2