Krdytkyu

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.

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.