Filtering users by criteria in Ruby on Rails

Question

I am a fairly new (RubyonRails) developer. I desire to improve my coding skills so I used climate to do some review on my code. It gave me a flag for this method that its complex. Is it characterised as complex because of having several "actions/tasks" in a single method?

Will it be better if I extract some code segments to a different method?

Is there something else I am not seeing?

 def search
 filter_mapping = {"cost" => "rcost", "quality" => "rquality", "time" => "rtime", "experience" => "rexperience", "communication" => "rcommunication"}
 @filters = params[:filter].split(",") rescue []
 @sort = params[:sort]
 @user_type = params[:s_user_type]
 @skills = params[:s_skills]
 @location = params[:location]
 @max_rate = params[:max_rate]
 @availability = params[:availability]
 @users = User.scoped.joins(:user_skills)
 @users = @users.where('user_type = ?', @user_type) if @user_type.present?
 @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0
 @users = @users.where('availability = ?', @availability) if @availability.present?
 @users = @users.where('location_country = ?', @location) if @location.present?
 @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?
 @users = @users.page(params[:page]).per(PER_PAGE)
 @filters.each do |f|
 if filter_mapping.keys.include?(f)
 @users = @users.order("#{filter_mapping[f]} desc")
 end
 end
 @users = @users.order('id asc') if @filters.empty?
 @advanced_link = @location.blank? && @max_rate.blank? && @availability.blank?
 render :index
 end

Update

I figured out that I can extract the scopes into a method like that:

 def get_users_where_scopes
 @users = User.scoped.joins(:user_skills)
 @users = @users.where('user_type = ?', @user_type) if @user_type.present?
 @users = @users.where('user_skills.skill_id in (?)', @skills.map(&:to_i)) if @skills.present? && @skills.size > 0
 @users = @users.where('availability = ?', @availability) if @availability.present?
 @users = @users.where('location_country = ?', @location) if @location.present?
 @users = @users.where('rate < ?', @max_rate.to_i) if @max_rate.present?
 @users = @users.page(params[:page]).per(PER_PAGE) 
 end

and then call it with @users = get_users_where_scopes(). But now the complexity of this method seems wrong to me.

Answer 1

I'd say to first make a service object to keep the controller lean and clean, and to give yourself a place to put all the logic without fear of polluting the controller. Plus: It's reusable!

# app/services/user_search.rb
class UserSearch
 ORDER_MAPPING = {
 "cost" => "rcost",
 "quality" => "rquality",
 "time" => "rtime",
 "experience" => "rexperience",
 "communication" => "rcommunication"
 }.freeze
 def initialize(params)
 @params = params
 end
 def results
 @results ||= begin
 records = User.scoped.joins(:user_skills)
 records = scope(records)
 records = order(records)
 end
 end
 private
 def param(key)
 @params[key] if @params[key].present?
 end
 def scope(scoped)
 scoped = add_scope(scoped, 'user_type = ?', param(:user_type))
 scoped = add_scope(scoped, 'user_skills.skill_id in (?)', skill_ids)
 scoped = add_scope(scoped, 'availability = ?', param(:availability))
 scoped = add_scope(scoped, 'location_country = ?', param(:location))
 scoped = add_scope(scoped, 'rate < ?', max_rate)
 end
 def add_scope(scope, sql, *params)
 scope.where(sql, *params) if params.all?(&:present?)
 scope
 end
 def order(scope)
 terms = sanitized_order_terms || default_order_terms
 terms.each { |term| scope.order(term) }
 scope
 end
 def sanitized_order_terms
 terms = param(:filter).try(:split, ",")
 terms = terms.map { |term| ORDER_MAPPING[term] }
 terms = terms.compact
 terms if terms.any?
 end
 def default_order_terms
 ["id asc"]
 end
 def skill_ids
 param(:s_skills).try(:map, &:to_i)
 end
 def max_rate
 param(:max_rate).try(:to_i)
 end
end

I've intentionally kept the pagination in the controller, as it's pretty independent of the scoping and ordering. However, it'd be simple to add as arguments to the #results method

In your controller:

def search
 @users = UserSearch.new(params).results.page(params[:page]).per(PER_PAGE)
 advanced_params = %w(location max_rate availability).map { |p| params[p] }
 @advanced_link = advanced_params.all?(&:blank)
 render :index
end

I'd probably pick a more direct way of determining the @advanced_link, such as sending an advanced parameter along, and simply looking at that instead of the implicit state you have now.

I have no idea what Code Climate thinks of the code above, but I imagine it'll be happier.

• tokland: what do you think of reusing the same name of variables to hold different values? for me it's a no-no...
• Flambino: @tokland If you're referring to the @advanced_link-thing in the controller, which is just me being lazy; I'll change that - thanks! But for the UserSearch class' methods it was a conscious choice. E.g. in #scope the scoped var does change, yes, but no more than it would if there was a self-modifying where! I could call on it. I just see it as breaking a where().where().where... chain up into single links. The value changes quantitatively, not qualitatively, so to speak. If it were anything else, I'd use different vars (like I should have done in the controller code)