# 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)

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.

As you have realized, working directly with SQL strings is cumbersome. I'd use Arel, it's not so hard. You can use a wrapper like this one (disclosure: I wrote it :)) to make it a bit more readable.

Now you could write something like this, it looks pretty declarative to me:

def self.search(options = {})
work_types = [:full_time, :contractor, :freelance]