Metrics / AbcSize Too High: Как уменьшить AB C в этом методе?

Question

Недавно я начал использовать Rubocop для «стандартизации» своего кода, и это помогло мне оптимизировать большую часть моего кода, а также научить Ruby «трюков». Я понимаю, что должен руководствоваться собственными суждениями и отключать Cops там, где это необходимо, но я застрял с приведенным ниже кодом:

def index
  if params[:filters].present?
    if params[:filters][:deleted].blank? || params[:filters][:deleted] == "false"
      # if owned is true, then we don't need to filter by admin
      params[:filters][:admin] = nil if params[:filters][:admin].present? && params[:filters][:owned] == "true"
      # if admin is true, then must not filter by owned if false
      params[:filters][:owned] = nil if params[:filters][:owned].present? && params[:filters][:admin] == "false"
      companies_list =
        case params[:filters][:admin]&.to_b
        when true
          current_user.admin_companies
        when false
          current_user.non_admin_companies
        end
      if params[:filters][:owned].present?
        companies_list ||= current_user.companies
        if params[:filters][:owned].to_b
          companies_list = companies_list.where(owner: current_user)
        else
          companies_list = companies_list.where.not(owner: current_user)
        end
      end
    else
      # Filters for deleted companies
      companies_list = {}
    end
  end
  companies_list ||= current_user.companies
  response = { data: companies_list.alphabetical.as_json(current_user: current_user) }
  json_response(response)
end

Среди прочего, я получаю следующую ошибку:

C: Metrics/AbcSize: Assignment Branch Condition size for index is too high. [<13, 57, 16> 60.61/15]

Я понимаю математику, лежащую в основе этого, но я не знаю, как упростить этот код для достижения того же результата.

Может кто-нибудь дать мне какое-то руководство по этому поводу? 1009 *

Заранее спасибо.

Answer 1

Ну, прежде всего, полностью ли протестирован этот код, включая все мириады условий? Это настолько сложно, что рефакторинг наверняка будет катастрофическим, если набор тестов не будет строгим. Итак, напишите комплексный набор тестов, если у вас его еще нет. Если уже существует набор тестов, убедитесь, что он проверяет все условия.

Во-вторых, примените парадигму «толстая модель и тощий контроллер». Итак, перенесите всю сложность в модель, назовем ее CompanyFilter

def index
  companies_list = CompanyFilter.new(current_user, params).list
  response = { data: companies_list.alphabetical.as_json(current_user: current_user) }
  json_response(response)
end

и переместим все эти операторы if / then / else в метод CompanyFilter#list

тесты все еще проходят? отлично, вы все равно будете получать предупреждения Rubocop, но относящиеся к классу CompanyFilter.

Теперь вам нужно распутать все условия. Мне немного сложно понять, что происходит, но похоже, что это должно быть сведено к одному оператору case с 5 возможными результатами. Итак, класс CompanyFilter может выглядеть примерно так:

class CompanyFilter
  attr_accessors :current_user, :params

  def initialize(current_user, params)
    @current_user = current_user
    @params = params
  end

  def list
    case
    when no_filter_specified
      {}
    when user_is_admin
      @current_user.admin_companies
    when user_is_owned
      # etc
    when # other condition
      # etc
    end
  end

  private
  def no_filter_specified
    @params[:filter].blank?
  end

  def user_is_admin
    # returns boolean based on params hash
  end

  def user_is_owned
    # returns boolean based on params hash
  end
end

тесты все еще проходят? идеальный! [Edit] Теперь вы можете перенести большинство тестов вашего контроллера в модельный тест для класса CompanyFilter.

Наконец, я бы определил все различные запросы companies_list как области на модели компании, например

class Company < ApplicationRecord
  # some examples, I don't know what's appropriate in this app
  scope :for_user, ->(user){ where("...") }
  scope :administered_by, ->(user){ where("...") }
end

Answer 2

При компоновке областей базы данных ActiveRecord :: SpawnMethods # merge - ваш друг.

Post.where(title: 'How to use .merge')
    .merge(Post.where(published: true))

Хотя это не так уж и много, но позволяет вам программно составлять области без чрезмерного изменения деревья присваивания и if / else. Вы можете, например, составить массив условий и объединить их в один объект ActiveRecord::Relation с помощью Array#reduce:

[Post.where(title: 'foo'), Post.where(author: 'bar')].reduce(&:merge)
# => SELECT "posts".* FROM "posts" WHERE "posts"."title" = $1 AND "posts"."author" = $2 LIMIT $3

Итак, давайте объединим это с подходом тонких контроллеров, где вы обрабатываете фильтрацию отдельно. object:

class ApplicationFilter
  include ActiveModel::Attributes 
  include ActiveModel::AttributeAssignment 
  attr_accessor :user

  def initialize(**attributes)
    super()
    assign_attributes(attributes)
  end

  # A convenience method to both instanciate and apply the filters
  def self.call(user, params, scope: model_class.all)
    return scope unless params[:filters].present?
    scope.merge(
      new(
        permit_params(params).merge(user: user)
      ).to_scope
    )
  end

  def to_scope
    filters.map { |filter| apply_filter(filter) }
           .compact
           .select {|f| f.respond_to?(:merge) }
           .reduce(&:merge)
  end

  private
  # calls a filter_by_foo method if present or 
  # defaults to where(key => value)
  def apply_filter(attribute)
    if respond_to? "filter_by_#{attribute}"
      send("filter_by_#{attribute}")
    else 
      self.class.model_class.where(
        attribute => send(attribute)
      )
    end
  end
  # Convention over Configuration is sexy.
  def self.model_class 
    name.chomp("Filter").constantize 
  end

  # filters the incoming params hash based on the attributes of this filter class
  def self.permit_params
    params.permit(filters).reject{ |k,v| v.blank? }
  end
  
  # provided for modularity
  def self.filters
    attribute_names
  end
end

Здесь используются некоторые возможности, предоставляемые Rails, для установки объектов с атрибутами, которые будут динамически обрабатывать атрибуты фильтрации. Он просматривает список объявленных вами атрибутов, а затем вырезает их из параметров и применяет метод для этого фильтра, если он присутствует.

Затем мы можем написать конкретную реализацию:

class CompanyFilter < ApplicationFilter
  attribute :admin, :boolean, default: false
  attribute :owned, :boolean 

  private

  def filter_by_admin
    if admin
      user.admin_companies
    else
      user.non_admin_companies
    end
  end

  # this should be refactored to use an assocation on User
  def filter_by_owned
    case owned
    when nil
      nil
    when true
      Company.where(owner: user)
    when false
      Company.where.not(owner: user)
    end
  end
end

И вы можете позвонить по нему с помощью:

# scope is optional
@companies = CompanyFilter.call(current_user, params), scope: current_user.companies)