重构这个控制器？

Question

class ArticlesController < ApplicationController

  def index
    @articles = Article.by_popularity

    if params[:category] == 'popular'
      @articles = @articles.by_popularity
    end

    if params[:category] == 'recent'
      @articles = @articles.by_recent
    end

    if params[:category] == 'local'
      index_by_local and return
    end

    if params[:genre]
      index_by_genre and return
    end

    respond_to do |format|
      format.html # index.html.erb
      format.xml  { render :xml => @articles }
    end
  end

  def index_by_local
    # 10 lines of code here

    render :template => 'articles/index_by_local'
  end

  def index_by_genre
    # ANOTHER 10 lines of code here

    render :template => 'articles/index_by_genre'
  end
end

正如你从上面看到的。我的控制器并不完全薄。它的作用是，根据传递的参数，它与模型交互以过滤掉记录。

如果传递了 params[:local] 或 params[:genre]。然后它分别调用自己的方法（def index_by_local和def index_by_genre）进行进一步处理。这些方法还加载自己的模板，而不是 index.html.erb。

这对于控制器来说是非常典型的吗？或者我应该以某种方式重构它？

Answer 1

我们可以将前几行移到模型（article.rb）中：

def get_by_category(category)
  # Return articles based on the category.
end

这样我们就可以使用单元测试来完整地测试文章获取逻辑。

一般来说，将与获取模型内的记录相关的所有代码移至模型中。
一般来说，控制器

1. 应该授权用户
2. 使用参数获取记录并将它们分配给实例变量
   [这些通常必须是功能
   调用模型]
3. 渲染或重定向

Answer 2

我将为您想要使用的每个集合定义范围。

class Article < ActiveRecord::Base
  ...
  scope :popular, where("articles.popular = ?", true) # or whatever you need
  scope :recent, where(...)
  scope :by_genre, where(...)
  scope :local, where(...)
  ...

  def self.filtered(filter)
    case filter
    when 'popular'
      Article.popular, 'articles/index'
    when 'recent'
      Article.recent, 'articles/index'
    when 'genre'
      Article.by_genre, 'articles/index_by_genre'
    when 'local'
      Article.local, 'articles/index_by_local'
    else
      raise "Unknown Filter"
    end
  end
end

然后在你的控制器操作中，像这样：

def index
  @articles, template = Article.filtered(params[:category] || params[:genre])
  respond_to do |format|
    format.html { render :template => template }
    format.xml  { render :xml => @articles }
  end
end