ruby-on-rails - rails : clean up messy controller methods

Question

我有一个很长的 Controller 方法，有很多重定向条件:

def show
  get_param_user
  if params[:id].match(/\D/)
    @document = Document.where(:user_id => @user.id, :issue => params[:id]).first
  else
    @document = Document.find(params[:id])
  end
  unless @document.blank?
    unless @document.template.name == "Media"
      unless @document.retired?
        @creator = User.find(@document.user)
        if @creator == @user # if document exists, based on name and id
          @document.components.each do |a|
            redirect_to share_error_url, :flash => { :error =>  "#{@document.title} contains retired content and is now unavailable." } if a.retired? and return
          end
          render @document.template.name.downcase.parameterize.underscore
        end
      else # if retired
        redirect_to share_error_url, :flash => { :error =>  "That document has expired." } and return
      end
    else # if media
      redirect_to share_error_url, :flash => { :error =>  "Media has no public link." } and return
    end
  else # if document doesn't exist
    redirect_to share_error_url, :flash => { :error =>  "Can't find that document. Maybe check your link. Or maybe it was deleted. Ask #{@user.name}." } and return
  end
end

正如您可能猜到的那样，在某些情况下它很容易出错。有没有更简洁的方法来重写它以使其更健壮？我知道方法应该只有一个 render或 redirect_to每个，但我不知道如何才能实现我的需要。

谢谢!

Answer 1

一些具体的小事情。

首先，一般来说，最好不要使用until与 else条件，如果您可以使用 if 甚至更少:

unless @document.blank?

是一样的

if @document.present?

其次，你使用

    @creator = User.find(@document.user)

通常你可以简单地使用:

    @creator = @document.user

语义有点不同(在第一种情况下，如果 @document.user 是 nil 你会立即得到一个异常，在第二种情况下不会)，但第二种情况是你通常需要的。

第三，如果合理，您可以将代码移到远离 Controller 的模型中，并使用一些不错的枚举器:

def has_retired_components?
  @document.components.any?(&:retired?)
end

此外，您的 Controller 方法并不复杂。这只是

if @document.present? and @document.showable? # also @document.try(:showable?)
  render whatever
else
  redirect_to error_url, flash: { error: error_message }
end

error_message可能是方法调用的结果(如果有意义的话，在对象本身上)。通过这种方式，您可以移动逻辑以验证对象是否可以在其他地方显示，在那里它与渲染逻辑不那么困惑。

问题是，如果您有 showable?方法和另一个显示错误消息的方法，您必须确保两者的业务逻辑始终正确。一种选择是将其与验证的工作方式类似地对待它:有一个方法(让我们用可怕的名称 showable_validation 在这里调用它只是继续)返回带有错误和消息的散列(对象无法显示的原因) , 像 {title: 'this is an error message'} . 那么 showable? 方法将是:

def showable?
  showable_validation.empty?
end

然后你也会在模型中拥有类似的东西:

def showable_error
  showable_validation.values.first
end

那将是 error_message ( @document.showable_error )。这样，逻辑就只有一种方法。