Don't be too DRY - Filter Abuse April 29th, 2008

Controller Filters are a great way to add behaviour to your controller which isn’t directly connected to your actions. The Hitchhiker’s Guide to the GalaxyRails Documentation has this to say on the subject of Filters

Filters enable controllers to run shared pre- and post-processing code for its actions. These filters can be used to do authentication, caching, or auditing before the intended action is performed. Or to do localization or output compression after the action has been performed. Filters have access to the request, response, and all the instance variables set by other filters in the chain or by the action (in the case of after filters). source

A good example to this is authentication. A controller action’s role isn’t authentication, but you should be sure that whoever reaches that action is authenticated. This can be solved with a simple before_filter

class PostsController < ApplicationController
  before_filter :check_auth, :except => [:show, :index]

  def index
    @posts = Post.all
  end

  def new
    @post = Post.new
  end 

  ...
  private
  def check_auth
    User.authenticate(session[:user]) || redirect_to login_path
  end
end

Lately, however, I’ve seen some very bad use of this functionality. We all like the Dont Repeat Yourself principle, but this shouldn’t interfere with code readability. Actions should be descriptive. When reading an actions code, you should know what it does.

This is bad:

class PostsController < ApplicationController
  before_filter :find_post, :only => [:show, :edit, :update]
  before_filter :all_posts, :only => [:index]
  before_filter :find_comments, :only => [:show]

  def index
  end

  def show
  end

  def edit
  end

  def update
    @post.update_attributes(params[:post)
    redirect_to @post
  end
  ...
  private
  def find_post
    @post = Post.find(params[:id])
  end

  def all_posts
    @posts = Post.find(:all)
  end

  def find_comments
    @comments = Comment.find_by_post_id(params[:id])
  end
end

The reason people use filters this way is to be ultra DRY. You don’t need to do Post.find(params[:id]) 3 times in your controller, and it works this way.

I personally hate this pattern, and everyone who uses it. This code may be DRY, but it’s not readable at all.

When I see an action with nothing in it, and I look at the view template and see @post being used, I have to look back at the controller, look at which before_filters apply, find the methods called, interpret what is in there.

Other than that, it also removes the ability to do effective Action Caching, because action caching will interpret before_filters, thus running the SQL queries the find-methods trigger.

If you have too much code you’d have to replicate, there are other solutions. For example if you had

class PostController
  def show
    @post = Post.find(params[:id], :conditions => {:site_id => current_site.id, :deleted_at => nil}, :include => :comments)
  end

  def edit
    @post = Post.find(params[:id], :conditions => {:site_id => current_site.id, :deleted_at => nil}, :include => :comments)
  end

You should refactor this with a model-action instead of going for a before_filter. This could become

class Post
  named_scope :active, :conditions => {:deleted_at => nil}, :include => :comments
end

class PostController
  def show
    @post = current_site.posts.active.find(params[:id])
  end

  def edit
    @post = current_site.posts.active.find(params[:id])
  end
end

Believe me, readability is way more important then being DRY

tags: l 0 comments »