The good and bad of ActiveRecord callbacks

Callbacks inside your rails controllers have sparked a lot of controversy. On one side, you have DHH advocating for the use of callbacks to manage auxiliary complexity, and on the other hand, you have people that call it The biggest Rails code smell you should avoid. In this post I would like to analyze both sides of the argument and present an example of how callbacks were implemented in one area of our codebase and how it can benefit or hurt us.

Advantages

1) It allows to put complexity off in the side lanes: This allows developers to focus on the main flow of the logic in a straightforward way when we want to get a high-level understanding of the logic. Most of the time, if you are looking at your create method, you care about how the create action is being handled, not about the :welcome_email that is being sent out or the :notification that goes out to a receiver.

2) It "just works": Having callbacks that trigger side effects like sending out a notification, or triggering a background job allows allows your controller actions to be way more readable at first glance, since you don't have to figure out everything that is going on inside of it. And because you don't have to understand every intricacy of the flow, you can make changes more quickly in your app, assuming you follow the default path, this is the principle of convention over configuration. DHH actually frames this well in his 2018 RailsConf keynote as "Conceptual Compression".

Drawbacks

1) Unwanted side effects: Side effects that work are great when they do, but especially painful and hard to deal with when they don't. Following your normal flow might lead you to think a certain value is coming back but then getting something completely different is certainly frustrating.

2) Hard to opt out of: This is something that even DHH recognizes as a possible culprit on why callbacks have such a bad reputation, since this can easily come back to bite many developers in heavy callback scenarios. To counter this, he introduced the concept of supressed in the Basecamp codebase.

3) Tightly coupled actions: This is the one in particular I find most dangerous, using callbacks means that you are not able to test the callback actions on their own, since you need to call create or update on your object first which will set a cascade of callbacks. As an extreme, but very real example, you wouldn't be able to test your #send_receipt method without triggering #charge_credit_card which could go very wrong, very fast.

Example implementation:

Here is an example of our MatchOpportunities#update action:

class CatchOpportunitiesController < ApplicationController

  before_action :set_catch_opportunity
  ...

  def update
    if @catch_opportunity.update(catch_opportunity_params)
      if catch_opportunity_params[:interested_in_fish]
        unless catch_opportunity_params[:fisherman_profile_up_to_date]
          post_lack_of_interest_survey(
            catch_opportunity_params[:response_text],
            @catch_opportunity.id,
            'fisherman_profile_up_to_update'
          )
        end
      else
        catch_opportunity_params[:interested_in_fish]
        post_lack_of_interest_survey(
          catch_opportunity_params[:response_text],
          @catch_opportunity.id,
          'survey_results'
        )
      end
      MatchOpportunityMailer
        .with(catch_opportunity: @catch_opportunity)
        .fisherman_response_email
        .deliver_now
      render json: @catch_opportunity, status: 200
    else
      render json: @catch_opportunity.errors, status: 422
    end
  end

As you can see, this is A LOT of responsibilities for the #update method alone, leveraging the power of ActiveRecord callbacks, the new code would look something like this:

class CatchOpportunitiesController < ApplicationController

  after_action :send_email_with_fisherman_response,
                  only: :update, if: :profile_response_complete?
  after_action :post_fisherman_response, only: :update

  ...

  def update
    if @catch_opportunity.update(catch_opportunity_params)
      render json: @catch_opportunity, status: 200
    else
      render json: @catch_opportunity.errors, status: 422
    end
  end

  private

    def post_fisherman_response
      # handle posting to external service
    end

    def send_email_with_fisherman_response
      # handles email delivery
    end

    def profile_response_complete?
      # decides if callback should be performed at all.
    end

The second version of this snippet looks way more readable, just the fact that we managed to eliminate a double nested conditional is a big win, and although it is true that the #update method gives you no hint whatsoever that an email will get sent out, ultimately, that work is not hidden, it lets you know at the top of the file what action will be performed and where. Granted, this might not be the most perfect, beautiful, easy-to-test possible form of this code, but it's certainly an improvement on the original.

The fact that DHH, the creator of the Rails framework is in favor of using callbacks (mindfully), should be no surprise to anyone, since the rails framework, and the language ruby itself known to have a lot of magic (Implicit logic that allows you to get up and running faster, but hides complexity making the inner working of things not so obvious)

Personal Thoughts

There are some arguments against callbacks I didn't find very compelling. For example, stating that explicit code is always inherently better than implicit code is a bit of an overstatement in my opinion. Although some places like functional languages hold this idea as a very core part of their philosophy, Both Ruby and Rails are famously known for having a lot of syntactic sugar and embedded into it, and it's exactly that that helps people get up and running with them quickly.

I do see how this can be easily abused, and I find that the fact that it introduces tightly coupled actions that cannot run one without the other, and violating the Single Responsibility Principle of the controller, (which should only be concerned with persisting the data to the database). Charging a user's credit card when trying to test sending an email is an extreme, but real example.

At the end of the day, I see this as a display of Ruby's values: A very powerful tool, just as monkey patching, that gives you the freedom to build very powerful software, but can also screw you over very is used incorrectly. I don't believe it should be avoided at all costs, but like with most things, it has it's tradeoffs, so approach with caution.