How to avoid mounting SQL by concatenating strings?

I found myself mounting a query by concatenating strings. This just seems wrong. Any ideas of how to avoid this kind of situation?

I'm using Rails 4, and I don't see how can Arel make things better for my case.

def self.search(params)
    # query example
    # SELECT "jobs".* FROM "jobs"  WHERE (telecommute = 'f' AND ( freelance = 't' ) AND country = 'German')
    query = 'telecommute = ?'
    args = [params[:telecommute] == 'true']

    work_types = %W(full_time contractor freelance)
    accepted_work_types = params.find_all{ |k,v| work_types.include?(k) }.map{ |k,v| k } 
    if accepted_work_types.any?
      query += " AND ( #{accepted_work_types.map{ |wt| "#{wt} = ?"}.join(" OR ")} )"
      args += accepted_work_types.map{ |wt| true }
    end 

    [:city, :state, :country].each do |index|
      unless params[index].blank?
        query += " AND #{index} = ?"
        args << params[index]
      end 
    end 

    Job.where(query, *args)
  end

Yeah, that does feel kinda wrong. But OR queries in Rails are tricky.

There are two things here: One is creating the query, the other is parsing the params.

I'd probably start1 by preparing/parsing the params so building the query is cleaner. I'd add a method - or several - to the controller, rather than the model, to do this. Both model and controller are involved in this, but I'd rather not pass params to the model. Things like checking the string value of the telecommute param seems like a job for the controller, whereas the model's search method should only have to deal with a boolean. For instance, you might want to call Job.search from code, where it'd be silly to use string values.

There's some cleanup you can do here too. For instance, I'd strongly consider adding constants or methods for the work_types array and the location keys.

I'd also be more consistent; in one place you use %w(...) to make an array of (string) keys, and in another you use [...] to make an array of (symbol) keys.

Here's an example, if you were to just use one method:

def search_params
  search_terms = {}

  # put the "AND" terms in the hash
  search_terms[:telecommute] = params['telecommute'] == 'true'

  Job.LOCATION_KEYS.each_with_object(search_terms) do |key, memo|
    memo[key] = params[key] if key.present?
  end

  # put the "OR" terms in a nested hash
  Job.WORK_TYPES.each_with_object(search_terms) do |key, memo|
    if params[key].present?
      memo[:work_types] ||= {}
      memo[:work_types][key] = true
    end
  end

  search_terms
end

It ain't pretty, but at least it's all in one place.

As for the searching itself, you can use regular ActiveRecord's query interface without descending to Arel:

def self.search(hash = {})
  # we'll be modifying the hash, so work on a dup
  # (you'll first want to check if it's empty, though!)
  terms = hash.dup

  # grab the work types
  work_types = terms.delete(:work_types)

  # add the AND clauses
  query = self.where(terms)

  # add the OR clauses (if any)
  if work_types
    subquery = self.where(work_types).where_values.inject(:or) # magic!
    query = query.where(subquery)
  end

  query
end

I get this:

Job.search(telecommute: true, country: "US", work_types: { full_time: true, freelance: true }).to_sql
# => "SELECT "jobs".* FROM "jobs"  WHERE "jobs"."telecommute" = 't' AND "jobs"."country" = 'us' AND (("jobs"."full_time" = 't' OR "jobs"."freelance" = 't'))"

1 Edit: Actually, that's not where I'd start if I were writing this from scratch. It is however where I started my rewrite, but that's because it was already there. Otherwise, I'd start by defining the search method as I'd want it to look and work - without worrying about the params -, and then worry about how to make the params fit.

StackExchange Code Review Q#66384, answer score: 4