总是有很多方法（尤其是使用 ruby​​ 时）。这是我的方法以及我认为可以改进的地方，但您的意见和偏好可能有所不同。此外，您可以决定忽略 Rubocop。此警告并不是说这是错误的代码，只是说也许可以写得更好。

原始代码

def destroy
  unless Current.user.default_entity.owner == Current.user ||
          Current.user.default_entity.user_privileged?('manage_contacts')

    redirect_back(fallback_location: contacts_path, alert: t('errors.no_access_rights')) and return
  end

  unless @contact.invoices.count.zero?
    redirect_back(fallback_location: contacts_path,
                  alert: t('errors.cant_delete_contact_with_invoices')) and return
  end

  unless @contact.offers.count.zero?
    redirect_back(fallback_location: contacts_path,
                  alert: t('errors.cant_delete_contact_with_offers')) and return
  end

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

变体 1

让我们摆脱重复 - 我们可以在出现问题时设置警报，如果我们设置了警报，我们就返回并重定向。

def destroy
  alert = nil
  unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    alert = t('errors.no_access_rights')
  end

  unless @contact.invoices.count.zero?
    alert = t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    alert = t('errors.cant_delete_contact_with_offers')
  end

  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

我们实际上并没有使代码更短或更少分支，但我们获得了一些见解 - 有查找可能问题的逻辑，并且此方法有两种结束方式 - 出现错误时进行 redirect_back，或者破坏联系。

变体 2

让我们将设置警报移至单独的私有方法，使操作更加简洁

def destroy
  return redirect_back(fallback_location: contacts_path, alert:) if alert

  @contact.destroy

  redirect_to contacts_path, status: :see_other
end

private

def alert
  unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    return t('errors.no_access_rights')
  end

  unless @contact.invoices.count.zero?
    return t('errors.cant_delete_contact_with_invoices')
  end

  unless @contact.offers.count.zero?
    return t('errors.cant_delete_contact_with_offers')
  end
end

这里的命名可能不是很好，但这只是快速的头脑风暴。

这再次增强了核心destroy方法的可读性，并且使得移动细节变得更加简单，从而导致：

变体 3

为什么我们要检查模型是否可以删除？控制器不需要知道这一点，这些知识可能存在于模型中。此外，ActiveRecord 模型有一种很好的原生方式来管理验证和错误，因此这将帮助我们制作更标准的 Rails 端点。此处供参考 - Rails 文档和Rails 指南

# app/models/contact.rb
class Contact < ApplicationRecord
  ...

  before_destroy :validate_destruction, prepend: true


  private

  def validate_destruction
    unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
      errors.add t('errors.no_access_rights')
      return false
    end

    unless invoices.count.zero?
      errors.add t('errors.cant_delete_contact_with_invoices')
      return false
    end

    unless offers.count.zero?
      errors.add t('errors.cant_delete_contact_with_offers')
      return false
    end
  end
end

# app/controllers/contracts_controller.rb

def destroy
  if  @contact.destroy
    redirect_to contacts_path, status: :see_other
  else
    redirect_back(fallback_location: contacts_path, alert:) if alert
  end
end

漂亮又干净。

我并没有真正测试过所有的代码，所以我不能保证它能完全按照这种方式工作，我只是想指出方向

一些一般的想法：

• 您可以自行决定控制器中驻留的逻辑量。但是，控制器中的代码更难测试和重用。将代码放在模型中，可以非常轻松地为具有不同状态的联系人编写单元测试，检查是否允许我们删除它。
• 甚至还有一种有点极端的方法——让你的控制器始终只对一个对象执行一个动作，并返回 ok 或 not ok 的响应。
• 询问“谁应该知道/做这件事？”。控制员是否应该知道联系人有发票？只有在必要时才知道。这里不需要。
• 您可以随时尝试移​​动代码，将其捆绑到函数/方法/帮助程序/... 中，然后看看感觉如何。 试验、尝试，并获得对良好代码的直觉。

Rubucop 的抱怨是正确的。循环复杂性实际上表明此代码存在一些问题。一般来说，如果您的方法包含多个显式返回，请停止并重做。

1. 授权很重要。请妥善对待授权。

像这样进行“内联”授权是一个非常糟糕的主意：

unless Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')
    alert = t('errors.no_access_rights')
end

• 当未经授权时，它不能正确保释并阻止进一步执行。
• 您正在重复授权逻辑。
• 你返回了错误的状态代码，所以客户端认为除了闪现消息之外一切都正常。这对可测试性来说非常糟糕。
• 有太多部分悬垂在模型之外，并且这三行代码违反了迪米特法则。

虽然最好的解决方案是添加一个授权层（例如 Pundit），但您至少应该借鉴 Pundit/CanCan 的做法，在请求未获得授权时引发异常，以确保它能被保释并且任何进一步的回调都会停止。

# app/errors/authorization_error.rb
class AuthorizationError < StandardError
end

class ContactsController
  # you probably want to reuse this for creating and editing
  before_action :authorize_contact, only: [:destroy]

  private

  def authorize_contact
    # This still stinks but fixing it completely is out of scope
    if entity_owner? || manage_contacts?
      @contact = Contact.find(params[:id])
    else
      raise AuthorizationError      
    end
  end

  def default_entity
    Current.user.default_entity
  end

  def entity_owner?
    default_entity.owner == Current.user
  end

  def manage_contacts?
   default_entity.user_privileged?('manage_contacts')
  end
end

class ApplicationController < ActionController::Base
  rescue_from AuthorizationError, with: :deny_access

  def current_user
    Current.user
  end 

  def deny_access
    # either redirect to the login or render an error page
    render plain: "Oh uh. You can't do that silly bear!",
      status: :forbidden
  end
end

作为额外的奖励，您可以在测试/规范中绕过 rescue_from 来断言/预期引发异常。

更好的方法是提取， Current.user.default_entity.owner == Current.user || Current.user.default_entity.user_privileged?('manage_contacts')这样您的控制器就不会对模型有太多了解。Pundit 使用 Policy 类来实现这一点，您可以使用 PORO 进行模拟。

2. 你让事情变得比应有的更难

不要仅仅假设它@contact.destroy会成功，而要检查返回值。如果销毁失败，这些restrict_with错误关联将填充错误对象。

就像创建/编辑对象时一样，您可以使用它们向用户提供反馈。不同之处在于，您想将它们推入闪存中（“警报”实际上只是闪存中的一个键），而不是将它们显示在表单上。

class ContactsController < ApplicationController
  before_action :authorize_contact, only: [:destroy]

  def destroy
    if @contact.destroy
      redirect_to contacts_path, status: :see_other
    else
      flash[:alert] = @contact.errors.full_messages.join(', ')
      redirect_back(fallback_location: contacts_path)
    end
  end
end

或者，你可以使用数组来代替闪存消息的不稳定字符串连接。

class ContactsController < ApplicationController
  before_action :authorize_contact, only: [:destroy]

  def destroy
    if @contact.destroy
      redirect_to contacts_path, status: :see_other
    else
      flash[:alert] = @contact.errors.full_messages
      redirect_back(fallback_location: contacts_path)
    end
  end

  # ...
end

然而，这将需要对布局进行一些更改以处理显示数组闪存消息，我将其留给读者。

您可以使用 before_action 或辅助方法重构 destroy 方法，该方法返回一个布尔值，指示是否应继续删除。您可以使用一个保护子句方法，如果删除被阻止，则返回 true，并在其中执行重定向。

before_action :authorize_destroy, only: :destroy
before_action :check_dependent_records, only: :destroy

def destroy
  @contact.destroy
  redirect_to contacts_path, status: :see_other
end

private

def authorize_destroy
  return if Current.user.default_entity.owner == Current.user ||
            Current.user.default_entity.user_privileged?('manage_contacts')

  redirect_back(fallback_location: contacts_path, alert: t('errors.no_access_rights')) and return
end

def check_dependent_records
  if @contact.invoices.exists?
    redirect_back(fallback_location: contacts_path, alert: t('errors.cant_delete_contact_with_invoices')) and return
  end

  if @contact.offers.exists?
    redirect_back(fallback_location: contacts_path, alert: t('errors.cant_delete_contact_with_offers')) and return
  end
end