current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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
share|improve this question

2 Answers 2

up vote 4 down vote accepted

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.

share|improve this answer
up vote 3 down vote

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]
  address_attrs = [:city, :state, :country]
  Job.where(
    (Job[:telecommute] == (options[:telecommute] == "true")) &
    (options.keys & work_types).map { |attr| Job[attr] == true }.reduce(:or) &
    options.slice(*address_attrs).map { |attr, value| Job[attr] == value }.reduce(:and)
  )
end
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.