Metrics/AbcSize 太高了：如何降低此方法中的 ABC？

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]

我理解它背后的数学原理，但我不知道如何简化这段代码以获得相同的结果。

有人可以给我一些指导吗？

提前致谢。

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 class.

相关

现在你需要理清所有的条件。我有点难以理解发生了什么，但看起来它应该可以简化为一个单一的案例陈述，有 5 种可能的结果。所以 CompanyFilter class 可能看起来像这样：

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

测试还在通过？完美的！ [编辑] 现在您可以将大部分控制器测试移动到 CompanyFilter class.

的模型测试中

最后，我会将所有不同的 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" =  AND "posts"."author" =  LIMIT

因此，让我们将其与您在单独对象中处理过滤的瘦控制器方法结合起来：

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)

Metrics/AbcSize 太高了：如何降低此方法中的 ABC？

Metrics/AbcSize Too High: How do I decrease the ABC in this method?

ruby

ruby-on-rails-5

rubocop