Refactor this controller?

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

As you can see from above. My controller is not exactly thin. What its doing is, depending on the params that were passed, it interacts with the model to filter out records.

And if params[:local] or params[:genre] was passed. Then it calls its own methods respectively (def index_by_local and def index_by_genre) to do further processing. These methods also load their own template, instead of index.html.erb.

Is this quite typical for a controller to look? Or should I be refactoring this somehow?

Arun Kumar Arjunan · Accepted Answer · 2011-06-29 01:56:49Z

0

We can move the first few lines into the model(article.rb):

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

In this way we can completely test the article fetching logic using unit tests.

In general move all the code related to fetching records inside model. Controllers in general

should authorize the user
get records using the params and assign them to instance variables [These must typically be function calls to model]
Render or redirect

answered Jun 29, 2011 at 1:56

Arun Kumar Arjunan

6,8571 gold badge34 silver badges36 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Midwire · Accepted Answer · 2011-06-29 20:08:56Z

I would define scopes for each of the collections you want to use.

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

Then in your controller action, something like this:

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

Collectives™ on Stack Overflow

Refactor this controller?

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related