current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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.

share|improve this question
add comment

1 Answer

up vote 0 down vote accepted

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.

share|improve this answer
    
what do you think of reusing the same name of variables to hold different values? for me it's a no-no... –  tokland Apr 23 at 8:39
    
@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) –  Flambino Apr 23 at 9:32
add comment

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.