An alternate approach to Avdi’s method of slimming down Rails controllers

I recently watched/read Avdi’s fantastic article Slimming down hefty Rails controllers AND models using domain model events on Ruby Tapas. PS – you should probably subscribe to Ruby Tapas.

While being amazed by Avdi’s refactoring skills and watching him plug in proven software development patterns, I couldn’t help but think about how I would have approached this problem.

Recently, I’ve been trying to be extremely adherent to “The Rails Way.” I’ve been using as few gems as possible, and just working with what Rails gives you out of the box. It’s been an eye-opening experience to say the least.

If you haven’t yet watched the episode, or read the accompanying article, essentially the problem is a long update method in the controller for a project management app. It has a bunch of nested conditionals with methods to send out updates via email or Websockets depending on what has been updated. Because so much of this depends on the logged-in user and other session-specific variables, breaking the functionality out into a “domain object” didn’t make a ton of sense.

What Avdi observed (pun intended) is that these are actually life-cycle events for a task and associated project.

Looking over this action again, we realize something: hidden in all these conditionals is a series of domain-model lifecycle events, each with different concrete actions which trigger on those events.

  1. There are some actions to perform anytime the task is successfully updated.
  2. There are actions to take when the task has moved from one project to another.
  3. There are actions to perform when the task is newly created.
  4. There are actions that happen when the task’s status has changed.
  5. There are actions for when the task has been reassigned.

He then proceeded to add observers to the Task model and subsequently send out notifications to listeners whenever a life-cycle event occurred.

Immediately, I see a lot more objects here. I see a Move, a StatusChange, and a Reassignment; all resources related to a Task. Only a traditional updating of attributes belongs in this method and controller.

As an aside, I’m not exactly sure why handling the case of a newly created task is in the update action of the controller instead of create, but I’ll assume there is something here that we don’t know. For simplicity, I’m just going to skip it.

Let’s start with some routes:

resources :tasks do
  resource :move, only: [:create]
  resource :status_change, only: [:create]
  resource :reassignment, only: [:create]

Now, instead of just hitting the update action, anytime something happens with a Task, we’ve got a very distinct route for each special case.

Because the controller in the example is just returning JSON, I’m going to assume that this is a Javascript front-end communicating with the Rails app. This would require a change in the front-end logic to POST to a subresource url instead of a PUT to the task url.

Here’s our first controller to handle the case of when a Task moves from one project to another.

# app/controllers/tasks/moves_controller.rb

class Tasks::MovesController < ApplicationController
  def create
    old_project_id = @task.project_id

    if @task.update_attributes params.require(:task).permit(:project_id)
      push_project_update old_project_id
      push_project_update @task.project_id

      respond_to do |format|
        format.json { render "show", status: :accepted }
      respond_with @task do |format|
        format.json { render @task.errors.messages, status: :unprocessable_entity }

Now, let’s examine what needs to happen when a status change occurs.

# app/controllers/tasks/status_changes_controller.rb

class Tasks::StatusChangesController < ApplicationController
  def create
    old_status = @task.status

    if @task.update_attributes params.require(:task).permit(:status)
      notifee = @task.readers(false) - current_user
      if notifee
        mail_completion_notice(notifee) if @task.status == Status::COMPLETED
        mail_uncomplete_notice(notifee) if old_status == Status::COMPLETED

      respond_to do |format|
        format.json { render "show", status: :accepted }
      respond_with @task do |format|
        format.json { render @task.errors.messages, status: :unprocessable_entity }

I’ll leave it to the reader to come up with their own implementation of a Tasks::ReassignmentsController but I think you can see how this method works.

I’d also challenge you to start thinking very strictly in terms of REST and “The Rails Way.” This codebase is the perfect example that rarely does an app just have basic CRUD needs. More often, there are special cases, and paths that require a different response despite technically falling under one of the basic CRUD actions.

Think about updating a User in your own codebase. Does the same thing happen when they update their password as when they update their name? Or, are you sending a email in once case and not in the other?

Instead of nesting conditionals inside of your update action in your UsersController could their be a Users::PasswordsController? Start flushing out more and more sub-resources with small controllers for these special cases.

It may not follow the classic patterns, but I’ve enjoyed this method of thinking recently.

I’d love to hear what you think as well!

  • Hassanin Ahmed

    I think this is a great approach if you can isolate all the actions and events.

    Sometimes you could be limited by the design like you could update several things about a task before saving it.

    Also another advantage of using the listeners like he did is that these actions can be reused in other events.

    Great article :thumbsup:

    • kylewiest

      Thanks Hassanin. I agree that the design is a limiting factor for sure. With a JS front-end I was thinking it was a situation of having in-place updating. If this was a “traditional” Rails app, you’d need a dedicated screen for each of these actions.

      For the case of a reassignment, I might be in favor of this. In other cases, I definitely would not be.

      Thanks again for reading, and for taking the time to share your thoughts.

  • Thanks for taking the time to post this!

    I don’t really see our approaches as being alternatives so much as differing in scale. What you’ve done here is a re-design, not a refactoring – as you note, it changes the behavior of the code, requiring a change to any HTTP clients.

    I’m very careful to separate refactorings from redesign, but it’s very often the case that my refactoring is done in order to support a future redesign 🙂

    • Thanks Avdi. Great point about the distinction between refactoring and redesigning. This would break any existing tests and should be considered a redesign, for sure.

      Thanks so much for reading, and thanks again for your original Ruby Tapas episode.

  • syntax error in your routes, line #2 should be only: [:create]

    • kylewiest


      • maybe even

        resources :tasks, module: :tasks do
        resource :move, only: :create
        resource :status_change, only: :create
        resource :reassignment, only: :create

        I seem to recall DHH going on record in a podcast as preferring your way too, i.e. new action => new controller. I wonder what Basecamp’s routes.rb looks like…

        By the way, this isn’t strict REST at all. I know the Rails guides suggest it is so, but all the dogmatically persnickety implementations of Fielding I’ve seen would simply POST to the /task/:id endpoint with the action to be taken encoded in the form data e.g. { “action”: “status_change” }, this being “control data” in the language of the original REST paper.

        However, and perhaps fortunately for our sanity, that isn’t how Action Pack does business, my point being that when you’ve said thinking very strictly in terms of REST and “The Rails Way”, those aren’t two simultaneously achievable ideals.

        • Thanks for this. I have not read the Fielding paper close enough to catch this distinction.

          Thanks again for the comment, and as a fellow cyclist, and runner of an online event registration service, it seems as though we’ve got a bit in common!

      • AMC

        He’s right. There should be a colon after “only”

        • Ah, gotcha. Didn’t realize I was missing the colon. Thanks to both.