Problems with typical Rails controllers and before actions

One of the most common practices when writing controllers in Rails is using before_actions to keep them DRY. Sure, repeating code is a bad practice and leads to maintenance nightmare, but what happens when the readability drastically deteriorates after making the code DRY to the max? Is it still worth it? Let's see how it applies to controllers, what are the consequences and possible solutions.

Anatomy of typical Rails controller

Let's take a look how typical Rails controller may look like:

class ArticlesController < ApplicationControler
  before_action :load_article, only: [:show, :edit, :update, :destroy]

  def index
    @articles = Article.all
  end

  def show
  end

  def new
    @article = Article.new
  end

  def create
    @article = Article.new(article_params)

    if @article.save
      redirect_to [:articles]
    else
      render :new
    end
  end

  def edit
  end

  def update
    if @article.update(article_params)
      redirect_to [:articles]
    else
      render :edit
    end
  end

  def destroy
    @article.destroy
    redirect_to [:articles]
  end

  private

  def load_article
    @article ||= Article.find(params[:id])
  end

  def article_params
    params.require(:article).permit(:title, :body, :author_ids)
  end
end

Looks pretty innocent, nothing fancy is going on here. The load_article method keeps the controller DRY so that we don't need to repeat loading an article in show, edit, update and destroy actions.

That's very similar what scaffold generator would create. Seems like it's also considered as The Rails Way best practice. What could possibly go wrong here?

Let's see if we can make it a bit more complex. The articles can have many authors, so we can add loading authors in before actions to still keep the controller DRY.

class ArticlesContrller  < ApplicationController
  before_action :load_authors, only: [:new, :create, :edit, :update]

# other actions

  private

  def load_authors
    @authors ||= Author.all
  end
end

Is there any problem with such code? If we take a look at the views we will see some instane variables: @article and @authors, now let's take a look e.g. at editaction:

def edit
end

Any idea where the instance variables come from? Nothing is in the action body. Well, we can of course scan the entire controller looking for proper before action, then go to private method etc., but it's not really ideal. The method is broken into several pieces which are all around controller making it difficult to follow. That example is trivial and already causes some problems. Can you imagine a controller with multiple before actions with only and except options where the the views for creating and editing record are totally different? Good luck with understanding where all the instance variables come from in each action.

Abusing before actions even more

There are actually even more nasty examples of misusing before actions - modifying params. Imagine that the article can have multiple owners that are selectable in the views, but we also want to add current user to be the owner of the article when creating a new one. To avoid adding some more logic in create method body for whatever reason (maybe to comply with some gems that drive the controller's method flow which happens quite often) or extracting the logic to some proper object (Fear Of New Classes) someone decides to add current user to params in before action:

class ArticlesController < ApplicationController
  # multiple before actions
  before_action :add_current_user_as_owner, only: [:create]
  # even more before actions

  # proper actions

  private

  def add_current_user_as_owner
    # gotcha!
    owner_ids = params[:article][:owner_ids] << current_user.id
    params.deep_merge!(article: { owner_ids: owner_ids })
  end

  def article_params
    params.require(:article).permit(:body, :title, :author_ids, :owner_ids)
  end
end

Any idea how could the data in params in create action be different than the one sent with form? Good luck with debugging such issues, especially in legacy apps with huge controllers.

What are the before actions good for

The only justified usecase for before actions should be something that doesn't have any side effects concerning the state - example would be redirecting to sign in page when the user is not logged in. It doesn't brake the main flow of the action into several pieces, doesn't set any instance variables and works more like a guard clause.

Making controllers readable again

The proper fix should be to move all the logic from before actions to method body. Just don't fall into another trap where it's not clear where the instance variables come from. Such code is still better than before actions:

class ArticlesController < ApplicationController
  def show
    load_article
  end

  private

  def load_article
    @article ||= Article.find(params[:id])
  end
end

but it's not obvious what's available in views, setting some state in private methods is almost always a bad idea. Other semi-fix would be to use helper_method to indicate that something is going to be available in views:

class ArticlesController < ApplicationController
  helper_method :article

   def show
    load_article
   end

  private

  def article
    @article
  end

  def load_article
    @article = Article.find(params[:id])
  end
end

but it only looks nice if you have one action in controllers, for multiple actions it doesn't really solve the problem. If you want to keep the controllers readable you should make all the instance variables explicit:

class ArticlesController < ApplicationController
  def new
    @article = find_article
    @authors = find_authors
  end
end

Or pass the variables explicitly to the views:

class ArticlesContoller < ApplicationController
  def new
    render :new, locals: { article: find_article, authors: find_authors }
  end
end

which is actually the safest option: instance variables that are not set return nil, so you won't accidentally run into NoMethodError when dealing with non-existent instant variables.

Wrapping up

Before actions are misused in many situations: either for making the controller DRY or to make the methods look still like typical CRUD, even when it's not the case, which leads to serious deterioration in code readability. Fortunately, you can keep the code readable by setting the state explicitly in method's body.