Action packed controllers

Manfred Stienstra

04 May 2015

Feature churn can really get bogged down in older Rails projects. One of the reasons is action packed controllers: the art of stacking actions on a controller that should not have those actions. Let’s look at a few ways to identify these and get rid of them.

Sticking to guidelines

People have adopted a number of simple guidelines to keep the scope of controllers small. One of these is only using index, new, create, show, edit, update, destroy actions on the controller. When you need any actions other than these you’re probably packing on too much functionality.

resources :books do
  member do
    delete :delete_cover
  end
end

class BooksController < ApplicationController
  def new
    @book = Book.new
  end

  def delete_cover
    @book = Book.find(params[:id])
    if @book.cover.destroy
      redirect_to @book
    end
  end
end

A controller with an action that does not belong.

In the example above we’re packing functionality related to covers on top of the books controller.

Fixing the problem

Create a new controller specifically for the model you were operating on. Notice how this also cleans up the routes.

resources :books do
  resource :cover
end

class CoversController < ApplicationController
  def destroy
    @book = Book.find(params[:book_id])
    if @book.cover.destroy
      redirect_to @book
    end
  end
end

The CoversController operates on covers.

You might think: awesome, anyone can refactor a controller with an obvious solution but my controller is a cesspit worse than the atmosphere of Jupiter.

class BooksController < ApplicationController
  before_action :find_book_for_author, except: :edit_book
  before_action :disallow_offenders
  before_action :set_ccr_rules_header, only: :new
  layout :set_layout_for_author

  def delete_cover
    if @book.cover.destroy
      redirect_to @book
    end
  end

  private

  def find_book_for_author
    @author = Author.find(session[:current_author_id])
    @book = @author.books.find(param[:id])
  end

  # […] etc etc
end

Refactoring always happens with baby steps. The solution here is to embrace the cesspit and think about fixing it later.

# Contains all methods shared by the books
# and covers controller.
module Concerns::BooksAndCoversControllerMethods
  protected

  def find_book_for_author
    @author = Author.find(session[:current_author_id])
    @book = @author.books.find(book_id_param)
  end

  # […] etc etc
end

class CoversController < ApplicationController
  include Concerns::BooksAndCoversControllerMethods

  before_action :find_book_for_author
  before_action :disallow_offenders
  layout :set_layout_for_author

  def destroy
    if @book.cover.destroy
      redirect_to @book
    end
  end

  private

  def book_id_param
    params[:book_id]
  end
end

Shared callbacks are grouped in a concern.

Even though all the ugly before actions are still there we’ve constrained them to one source file. Our original goal of simplifying the books controller and the routes has been achieved.

Usually when you split out code like this you will get some ideas on how to refactor it further. Don’t worry if you don’t have an epiphany immediately. You can leave the code and think about a great solution in the shower.