Controller Best Practices: Don’t Hide Instance Variables

Last Updated on May 30, 2023

This is something I often mention in code reviews, and it’s also mentioned in some Rails guidelines elsewhere without further explanation: please, don’t hide instance variables. Everyone who has dealt with controllers full of filters and indirections has good reasons to complain about this, for it’s DRY badly applied. Why make developers lives harder?

A typical Rails controller

Rails has chosen to use instance variables, and thus meta programming, to solve the problem of exposing values from controller actions to view templates. It works succinctly, but it has some potential problems.

The practical consequence of this fact is that we shouldn’t use ivars the same way we typically use them in other Ruby objects (although we still can). Besides that, controllers are designed to not follow SRP, which makes it even harder to understand the role of arbitrary ivars buried deep down beneath controller classes.

Here’s a typical kind of Rails controller:

class ThreadPostsController < ApplicationController
  before_action :find_thread, only: [:show, :edit, :update, :destroy]
  before_action :find_issue
  before_action :find_all_issues, only: [:index]
  before_action :extract_authentication_params, only: [:create], if: -> { params[:thread].present? }
  before_action :authenticate_anonymous_user, only: [:create]
  before_action :initialize_edit, only: [:edit, :update]
  before_action :find_parent, only: [:edit]
  before_action :hide_issue_history, only: [:show]
  before_action :validate_parent_id, only: [:new]

  def edit
    authorize @thread
  end

  # many other actions here...

  private

  # many other private methods here...

  def find_thread
    @thread = ThreadPost.find(params[:id])
  end

  # many other private methods here...
end

Now, imagine this controller has hundreds of lines, a bad practice in itself. Good questions may come up when you see this kind of code:

Can you tell which filters assign which instance variables to the edit template? You can’t, unless you scroll down and look at each filter method individually.
Can you easily understand which dependencies exist between filters? Usually there is some kind of weird dependency between filters, established by instance variables which you haven’t got a clue where are defined.
Can you easily tell which instance variables get assigned to the edit template? No, you can’t! Good luck trying to figure that out.

Now, wouldn’t it be great if you looked down at the edit method, one single spot, and could instantly tell what the view contract is?

Each view has a contract, which is carried out by its action

Each top level view template is coupled to a set of instance variables, which we call a contract. When you hide instance variables behind filters or private methods you make the life of anyone who’s trying to understand that contract harder.

One has to mentally compile filter methods, one by one, to understand which instance variables any template takes in. That means going back and forth many times when reading the code, because there isn’t a straightforward way to tell which filter assigns which instance variables to which actions— unless there is a rigid ivar/filter name convention in place, which there usually isn’t. Even if there is, it still gets tricky anyway.

And not only that: one also has to hop into the action and other private methods to catch out any other lost pesky ivars––and mentally form the contract picture from the puzzle pieces.

Imagine how frustrating this can be on a messy controller, which is unfortunately what we find a lot in real world code. That’s not to mention other hideousness like hiding instance variables behind concerns or subclasses.

This bad style is easier to follow on small controllers, but even still there is a better way.

Best practice: only assign instance variables in controller actions

That means not assigning them anywhere else. Here’s how a refactored edit action might look like:

class ThreadPostsController < ApplicationController
  before_action :authenticate_anonymous, only: [:create]

  def edit
    @thread = find_thread(params[:id])
    authorize @thread

    @issue = find_issue(@thread)
    @parent_thread = find_parent(@thread)
  end

  # many other actions here...

  private

  # many private methods here...

  def find_thread(id)
    ThreadPost.find(id)
  end

  # many private methods here...
end

What’s the difference?

Instance variables are only getting assigned in the controller action method body.
We are still using private methods, but they now return values instead of hiding instance variables. That way we still preserve the DRYness of the code, and plus the contract gets visible at a glance to the reader.
We are following a slightly more functional style, and explicitly handling out variables to private methods––even though they are available globally as instance variables. That makes private method dependencies explicit, and the controller code gets easier to follow. Note that controllers are one of the few Rails artifacts which benefit from this style.

That also means repeating the ivar assignment in every action in which the ivar gets used, a very minor but beneficial kind of repetition. We have no mystery guests anymore, and plus the independent nature of controller actions springs up to evidence.

Should I stop using before actions?

Even though before actions are useful, I recommend you don’t use them to setup logic that’s central to your controller.

You should use the feature sparingly, and mostly for global logic that is not particularly important to understand business logic of the controller at hand. That usually is the case when logic needs to be shared between more than one controller, such as authentication logic.

Instance variables are OK, but you can use locals

You can get rid of instance variables and use locals instead. I’m not too radical about totally getting rid of instance variables, but locals are still a viable alternative. Just render your view explicitly and handle out the values it needs:

class ThreadPostsController < ApplicationController
  before_action :authenticate_anonymous, only: [:create]

  def edit
    thread = find_thread(params[:id])
    authorize thread

    render :edit, locals: {
      thread: thread,
      issue: find_issue(thread),
      parent_thread: find_parent(thread)
    }
  end

  # many other actions here...

  private

  # many private methods here...
end

There are additional benefits to this style. Your code will fail in an obvious way if you forget to pass any local variable within the locals hash, whereas with instance variables the errors will get silenced or obscured away. Every Rubyist has seen messages like this:

undefined method foo’ for nil:NilClass

That’s an unfortunate fact about how instance variables work in the Ruby language, because uninitialized ivars are always assumed to be nil.

Avoid mystery guests

The changes advocated in this blog post are subtle, but make a great deal of difference. It’s always better to start out with the best practice, independent of the controller size you’re dealing with.