我怎样才能使这个控制器方法不那么重？

Question

假设我有以下控制器和包含的方法。我觉得这个控制器方法（列表）太重了。我应该如何拆分这个？什么应该移到视图层，什么应该移到模型层？

注意：返回格式（包含 :text、:leaf:、id、:cls 的哈希）包含与 ProjectFile 数据库表中不同的字段，所以这让我质疑这个控制器方法实际上应该移动到多少模型层。

class ProjectFileController < ApplicationController
  before_filter :require_user

  def list
    @project_id = params[:project_id]
    @folder_id = params[:folder_id]

    current_user = UserSession.find
    @user_id = current_user && current_user.record.id

    node_list = []

    # If no project id was specified, return a list of all projects.
    if @project_id == nil and @folder_id == nil
      # Get a list of projects for the current user.
      projects = Project.find_all_by_user_id(@user_id)

      # Add each project to the node list.
      projects.each do |project|
        node_list << {
          :text => project.name,
          :leaf => false,
          :id => project.id.to_s + '|0',
          :cls => 'project',
          :draggable => false
        }
      end
    else
      # If a project id was specfied, but no file id was also specified, return a
      # list of all top-level folders for the given project.
      if @project_id != nil and @folder_id == nil
        # Look for top-level folders for the project. 
        @folder_id = 0
      end

      directory_list = []
      file_list = []

      known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl']

      # Get a list of files by project and parent directory.
      project_files = ProjectFile.find_all_by_project_id(@project_id,
                                                         :conditions => "ancestry like '%#{@folder_id}'",
                                                         :order => 'name')

      project_files.each do |project_file|
        file_extension = File.extname(project_file.name).gsub('.', '')

        if known_file_extensions.include? file_extension
          css_class_name = file_extension
        else
          css_class_name = 'unknown'
        end

        # Determine whether this is a file or directory.
        if project_file.is_directory
          directory_list << {
            :text => project_file.name,
            :leaf => false,
            :id => @project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        else
          file_list << {
            :text => project_file.name,
            :leaf => true,
            :id => @project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        end
      end

      node_list = directory_list | file_list
    end

    render :json => node_list
  end
end

Answer 1

您的用户模型中应该有一个关联，例如

has_many => :projects

所以你可以得到一个项目对象的数组

current_user.projects

而不是这个：

projects = Project.find_all_by_user_id(@user_id)

这意味着您也不需要检索 user_id。

您还可以将所有用于填充 node_list 的逻辑放入您的用户模型中。只需放入您的模型：

def node_list(project_id, folder_id=0)
  if @project_id == nil
    list = self.projects.map do |project|
      {
        :text => project.name,
        :leaf => false,
        :id => project.id.to_s + '|0',
        :cls => 'project',
        :draggable => false
      }
    end
  else
    ... # the rest of your code here, etc
  end
  return list
end

请注意，如果 folder_id 为 nil 并设置为零，这也可以删除您的检查，因为 def node_list(project_id, folder_id=0) 中的 folder_id=0 会自动执行此操作。

那么您的控制器将如下所示：

def list
  @current_user = UserSession.find
  render :json => @current_user.node_list(params[:project_id], params[:folder_id])
end

还有：

既然您只是将 directory_list 和 file_list 组合在一起，为什么不将 if-else 语句组合成：

{
  :text => project_file.name,
  :leaf => project_file.is_directory,
  :id => @project_id + '|' + project_file.id.to_s,
  :cls => css_class_name
}

如有必要，您可以随时对数组进行排序。

Answer 2

@乍得，

我认为你已经从我之前发帖的人那里获得了重构代码的好例子。

您似乎有足够的能力编写 ruby​​ 代码，所以我将尝试从“如何重构”的角度回答您的问题。

• 保持控制器精简

  • 在适用的情况下在过滤器之前使用
  • 每个控制器端点（动作）都是控制器类中的一个方法，它应该有一个“单一”的职责。
  • 您的“列表”方法应该找到相关数据（使用对适当模型类的方法调用）并让 Rails 进行默认渲染。您正在返回 json，因此只需一行即可。
  • 现在您需要在模型上最多调用一个方法，在特殊情况下需要调用两个方法。
  • 我为我想出了这句格言“如果动作方法超过 10 行，那就错了”。

• 让模特发胖

  • 将您的问题分解为更小的问题。
  • 您的 list 方法的代码分支过多。
  • 也许在高层次上，您可能希望将其分解为模型中的“语义”控制器类型方法，该方法主要处理 if then else。
  • 其他较小的方法可以完成剩下的工作。
  • 模型方法也应该有一小部分参数。参数太多，很脆，没有参数意味着它与太多的状态绑定。
  • 带有 1 或 2 个参数的方法很好。您可以拥有一个带有 3 个选项参数的方法。任何超过 3 个参数都适合重构。
  • 当前用户对象可以传入模型中，这没有什么问题。
  • 或者通过在其中添加更多方法来丰富您的用户模型。 （has_many :projects 然后使用协会提供的方法）。
  • 我始终随身携带关联文档。
  • 我总是随身携带 enumerables 文档。
  • 了解在 Enumerables 中映射、选择、拒绝和查找的功能。他们是你的朋友！

祝你好运！ :)

Answer 3

我认为您可以将大部分逻辑放入您的模型 ProjectFile 或任何合适的模型名称中：

ProjectFile < ActiveRecord::Base
  def node_list(project_id, folder_id, user_id)
    node_list = []

    # If no project id was specified, return a list of all projects.
    if project_id == nil and folder_id == nil
    # Get a list of projects for the current user.
    projects = Project.find_all_by_user_id(user_id)

      # Add each project to the node list.
      projects.each do |project|
        node_list << {
          :text => project.name,
          :leaf => false,
          :id => project.id.to_s + '|0',
          :cls => 'project',
          :draggable => false
        }
      end
    else
      # If a project id was specfied, but no file id was also specified, return a
      # list of all top-level folders for the given project.
      if project_id != nil and folder_id == nil
        # Look for top-level folders for the project. 
        folder_id = 0
      end

      directory_list = []
      file_list = []

      known_file_extensions = ['rb', 'erb', 'rhtml', 'php', 'py', 'css', 'html', 'txt', 'js', 'bmp', 'gif', 'h', 'jpg', 'mov', 'mp3', 'pdf', 'png', 'psd', 'svg', 'wav', 'xsl']

      # Get a list of files by project and parent directory.
      project_files = ProjectFile.find_all_by_project_id(project_id,
                                                     :conditions => "ancestry like '%#{folder_id}'",
                                                     :order => 'name')

      project_files.each do |project_file|
        file_extension = File.extname(project_file.name).gsub('.', '')

        if known_file_extensions.include? file_extension
          css_class_name = file_extension
        else
          css_class_name = 'unknown'
        end

        # Determine whether this is a file or directory.
        if project_file.is_directory
          directory_list << {
            :text => project_file.name,
            :leaf => false,
            :id => project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
        }
        else
          file_list << {
            :text => project_file.name,
            :leaf => true,
            :id => project_id + '|' + project_file.id.to_s,
            :cls => css_class_name
          }
        end
      end

      node_list = directory_list | file_list
    end
end

然后将node_list 分解为更易于管理的方法。我上面定义的方法很长，而且会做很多事情（内聚度低），因此将其分解将有助于克服这些缺点。

在您的控制器中，您可以这样称呼：

class ProjectFileController < ApplicationController
  before_filter :require_user

  def list
    @project_id = params[:project_id]
    @folder_id = params[:folder_id]

    current_user = UserSession.find
    @user_id = current_user && current_user.record.id

    nodes = node_list(@project_id, @folder_id, @user_id)
    render :json => nodes
  end
end

现在您的控制器更易于阅读，业务逻辑也被提取出来。这遵循了“瘦控制器，胖模型”的口头禅。