Search code examples
ruby-on-railsrubyrubocop

Cyclomatic complexity for index is too high


I have index action in my Controller:

def index
  authenticate_admin!
  @users = User.paginate(per_page: 25, page: params[:page])
  if params[:list_type].to_i == 2 # => Pending mentors
    @users = @users.where(documents: { is_verified: 0 }, user_type: 0, profile_status: 1)
  elsif params[:list_type].to_i == 1 # => approved listing
    @users = @users.where(documents: { is_verified: 1 }, user_type: 0) if params[:user_type].to_i.zero? # => Mentor
    @users = @users.where(user_type: 1) if params[:user_type].to_i == 1 # => mentee
  end
  @users = @users.where('full_name LIKE ?', "%#{params[:search]}%") if params[:search].present?
  @users = if params[:sort].to_i == 1
    @users.order(full_name: :asc)
  else
    @users.order(id: :desc)
  end

  @users = @users.profession.where(id: params[:profession_ids]) if params[:profession_ids].present?
  @users = @users.call('age >= ?', params[:from_date]) if params[:from_date].present?
  @users = @users.call('age <= ?', params[:to_date]) if params[:to_date].present?
  @users = @users.where(gender: params[:gender]) if params[:gender].present?
  @users = @users.includes(:document)
end

but when I run Rubocop to check if my code has any offense. then it returns me two errors.

app/controllers/users_controller.rb:5:3: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for index is too high. [11/6]
  def index ...
  ^^^^^^^^^
app/controllers/users_controller.rb:5:3: C: Metrics/PerceivedComplexity: Perceived complexity for index is too high. [13/7]
  def index ...
  ^^^^^^^^^

So, I divide my index action to multiple actions like:

def index
  authenticate_admin!
  @users = User.paginate(per_page: 25, page: params[:page])
  if params[:list_type].to_i == 2 # => Pending mentors
    @users = @users.where(documents: { is_verified: 0 }, user_type: 0, profile_status: 1)
  elsif params[:list_type].to_i == 1 # => approved listing
    @users = @users.where(documents: { is_verified: 1 }, user_type: 0) if params[:user_type].to_i.zero? # => Mentor
    @users = @users.where(user_type: 1) if params[:user_type].to_i == 1 # => mentee
  end

  search
end

def search
  @users = @users.where('full_name LIKE ?', "%#{params[:search]}%") if params[:search].present?
  @users = if params[:sort].to_i == 1
    @users.order(full_name: :asc)
  else
    @users.order(id: :desc)
  end

  apply_filter
end

def apply_filter
  @users = @users.profession.where(id: params[:profession_ids]) if params[:profession_ids].present?
  @users = @users.call('age >= ?', params[:from_date]) if params[:from_date].present?
  @users = @users.call('age <= ?', params[:to_date]) if params[:to_date].present?
  @users = @users.where(gender: params[:gender]) if params[:gender].present?
  @users = @users.includes(:document)
end

Is this the right way to implement code like this or is there anything else i can do to refine my coding structure?


Solution

  • Is this the right way to implement code like this

    As long as you find the code reasonable and clear. If the index method is the only method in the controller, then it won't be a problem to navigate through the logic of the users search.

    is there anything else i can do to refine my coding structure

    One of the options would be extracting the these two methods (or even more logic) into a separate class/file. For example, something like:

    controller

    def index
      authenticate_admin!
      @users = User.paginate(per_page: 25, page: params[:page])
      if params[:list_type].to_i == 2 # => Pending mentors
        @users = @users.where(documents: { is_verified: 0 }, user_type: 0, profile_status: 1)
      elsif params[:list_type].to_i == 1 # => approved listing
        @users = @users.where(documents: { is_verified: 1 }, user_type: 0) if params[:user_type].to_i.zero? # => Mentor
        @users = @users.where(user_type: 1) if params[:user_type].to_i == 1 # => mentee
      end
    
      @users = Users::Search.new(@users).call
    end
    

    service

    class Users::Search
      attr_reader :params, :users
    
      def initialize(users, params)
        @users = users
        @params = params
      end
    
      def search
        users = @users.where('full_name LIKE ?', "%#{params[:search]}%") if params[:search].present?
        users = if params[:sort].to_i == 1
          users.order(full_name: :asc)
        else
          users.order(id: :desc)
        end
    
        apply_filter(users)
      end
    
      def apply_filter(users)
        users = users.profession.where(id: params[:profession_ids]) if params[:profession_ids].present?
        users = users.call('age >= ?', params[:from_date]) if params[:from_date].present?
        users = users.call('age <= ?', params[:to_date]) if params[:to_date].present?
        users = users.where(gender: params[:gender]) if params[:gender].present?
        users.includes(:document)
      end
    end
    

    And one of the advantages of the extracting is that such class is easier to test, than a controller method, which requires a request and parsing a response for every test case.