Are filters abused in Rails?

alternate_method_controller.rb

class MyController < ApplicationController
  def show
    @model = load_model
  end

  def edit
    @model = load_model
  end

  private

  def load_model
    Model.find(params[:id])
  end
end

filter_controller.rb

class MyController < ApplicationController
  before_filter :load_model, :only => [:show, :edit, :update, :destroy]

  def show
  end

  def edit
  end

  private

  def load_model
    @model = Model.find(params[:id])
  end
end

helper_method_controller.rb

class MyController < ApplicationController
  helper_method :model

  def show
    # Use +model+ freely
  end

  def edit
    # Use +model+ freely
  end

  private

  def model
    @model ||= Model.find(params[:id])
  end
end

method_controller.rb

class MyController < ApplicationController
  def show
    load_model
  end

  def edit
    load_model
  end

  private

  def load_model
    @model = Model.find(params[:id])
  end
end

I've been trying to remember a time when I've been bitten by not knowing how an instance variable has been defined in a controller and I can't think of one. So by default I think I'd probably opt for the before_filter option. It's enough of a rails idiom that it doesn't take too much knowledge to figure out what's going on. This is especially the case if you have a FoosController which does Foo.find: anyone with any Rails experience will know this pattern. It's even the basis of gems like inherited_resources.

I'm coming round to the idea that application_controller shouldn't define instance variables. Instead you could just use regular methods and make them helper_methods if you need them in views. That would at least remove some of the dangers of accidentally overwriting instance variables through inheritance.

The single action controller idea does sound interesting, but I'm not sure I agree with @lucabelmondo that the filter behaviour can be easily captured by the initialize method. With the initialize method you lose the ability to skip filters, which means each action would probably have to define each filter on every action explicitly. At some point, you would have loads of duplication in your actions and you'd probably start to abstract some of the more common ones. And then you're reinventing filters! So maybe the skip_before_filter pattern is the flaw. But I can't think of a way of removing skip_before_filter without introducing lots of duplication elsewhere.

We've been bitten by overzealous filters in ApplicationController recently (e.g. redirecting users to a mandatory landing page if they haven't seen it before interfering with API calls, etc.) and maybe that's the real problem here: a sort of action at a distance. Perhaps having a policy of not setting filters in superclasses is enough to keep the filter chain obvious?

The problem with this is that it is nice to use the following pattern:

class Admin::AdminController < ApplicationController
  before_filter :admin_required
end

class Admin::FoosController < Admin::AdminController
  # Every action is automatically filtered
end

I guess for me it's not really action at a distance. It's really just a constraint on your app, in the same way that auth or logging or other cross-cutting concerns are. So to me that does feel like it's a good candidate for a global filter. When you write controller tests for a new action it'd fail at first, and that's when you'd have to decide whether to skip it or not. That's good, because it forces you to consider the landing page every time you write a new action. I suppose you could also mitigate it by only redirecting to the landing page on GET requests and for :html rather than :js/:xml formats.

It's also interesting to see how Ramaze approaches the problem, namely relying on calls to super http://ramaze.net/documentation/file.controllers.html
I make no mystery of my predilection for Ramaze over Sinatra.

@ohthatjames I agree that a test should fail in this case but I'm not sure about using the presence of a safety net as your sole way of informing developers of the design, viz. someone could ostensibly create a new action and you'd be entirely reliant on your test coverage to teach them how the system works. Can we not hope for greater clarity in the code itself?

I'm reminded of another comment of yours regarding avoiding deeply nested contexts in tests for this same reason: the lack of transparency about what set up is actually occurring.

Following the Ruby Rogues discussion about Objects in Rails, I've added another approach: using helper_method to avoid exposing instance variables altogether. This has the added benefit of working across views.

In the common quest of a more better approach, the helper methods are a quite a interesting approach.

The helper methods though, as intended by Rails, break the encapsulation.

They also represent a problem for the single responsibility, because their presence is not always required by the controller instance, but dictated by the needs of the template.

As "cross object" instance variables which belong to 2 different objects at the same time, they are somehow pseudo-global entities instead of being injected.

When it comes to testing, they still need to be stubbed out as part of the context.

Also, if they assign instance variables, the latter will be again available in the view, representing a further temptation that only the developer's discipline can prevent :-)

Unfortunately all this is due to the very nature of how Rails works.

Furthermore, it's also related to the legacy of the original ERB implementation from the standard library. An ERB object only accepts a Binding object, and raises an error if the argument is of a different type. That probably encouraged this historical instance variable based approach in Rails.

Erubis (the default ERB engine as of Rails 3.0) does better in this respect, because you can also pass a Hash to the result.

template = Erubis::Eruby.new("<%= omg %>, this is a <%= message %>!!!")
object_to_be_rendered = {omg: "ZOMG!", message: "different kind of template"}
template.result(object_to_be_rendered)
# => "ZOMG!, this is a different kind of template!!!"

This allows to decide to rely on only one simple context object for the view, and this encourages to build that object as dumb as possible, probably a presenter on top of a database-backed model in the Rails common scenario.

This approach seems closer to the one we find in the Javascript templating systems. For me it has the advantage that I can easily stub it out not as pseudo-global context (difficult) but as a parameter to pass to the subject under test (easy).

It's also interesting that in we can get something similar in Rails making explicit use of the :locals option

render :template => "my/template", :locals => {omg: "ZOMG!", message: "different kind of template"}

The issue is how would you deal with global data. Like menu/cart etc if not using before filter