Effective Rails - Part 1 - ActiveRecord Callbacks

By JC Grubbs
about 1 year ago
views

Effective Rails - Part 1 - ActiveRecord Callbacks

We work with Rails a lot here at DevMynd. We love Rails, but we’re also the first to admit that like any framework it has some tricky bits. Careful use of the framework (avoiding the parts that really cause trouble) can make building complex applications in Rails less frustration-prone. Apps become more resilient to change when these features are used cautiously. This is the first post in a series that will explore techniques for effectively using some of the often misunderstood and abused parts of Rails.

As it turns out, at least part of my issue with this has been handled in Rails 4 as observers are no longer part of the API. Callbacks however are still alive and kicking. My issue with callbacks is quite simple: their inherent value is to couple multiple behaviors together. While they can be extremely alluring as a way to ensure certain behaviors occur together, it is very easy to make a mess with them. Take a look at this code:

# app/models/order.rb
class Order < ActiveRecord::Base

  has_many :line_items
  has_many :coupons

  before_save :update_weight, :update_shipping_cost, :update_total

  def update_weight
    self.total_weight = line_items.pluck(:weight).inject(:+)
  end

  def update_shipping_cost
    self.shipping_cost = ShippingCalculator.get_cost(self.total_weight)
  end

  def update_total
    sub_total = line_items.pluck(:price).inject(:+)
    percent_off = coupons.max_by { |c| c.percent_off }
    discount = sub_total * percent_off
    self.total_cost = (shipping_cost + sub_total) - discount
  end

end

Seems harmless enough, but the issue here is that we have now coupled four behaviors together: updating the shipping cost, updating the total weight, updating the total price, and saving the record. The plot thickens because we have also introduced a dependency on the order in which these callbacks are processed - we can’t accurately calculate the total_cost until the total_weight and shipping_cost have been tabulated.

Let’s take a look at how we might re-factor this code and clean up our use of callbacks. One option is to cram all of this behavior into an overridden save method:

# app/models/order.rb
def save
  self.shipping_cost = ShippingCalculator.get_cost(self.total_weight)
  self.total_weight = line_items.pluck(:weight).inject(:+)
  sub_total = line_items.pluck(:price).inject(:+)
  percent_off = coupons.max_by { |c| c.percent_off }
  discount = sub_total * percent_off
  self.total_cost = (shipping_cost + sub_total) - discount
  super
end

This removes the dependency on the order in which the callbacks are called. However it does nothing for decoupling the behaviors and in some ways seems worse than the callback approach. I generally don’t like overriding ActiveRecord methods or attribute getters/setters as it ties us too closely to behavior we don’t control. Overriding library methods makes upgrading more risky as well.

Let’s try breaking up the behaviors into component methods.

# app/models/order.rb
class Order < ActiveRecord::Base

  has_many :line_items
  has_many :coupons

  before_save :update_totals

  def update_total
    self.total_cost = (shipping_cost + sub_total) - discount
  end

  def update_weight
    self.total_weight = line_items.pluck(:weight).inject(:+)
  end

  def update_shipping_cost
    self.shipping_cost = ShippingCalculator.get_cost(self.total_weight)
  end

  def sub_total
    line_items.pluck(:price).inject(:+)
  end

  def discount
    sub_total * coupons.max_by { |c| c.percent_off }
  end

  def update_totals
    update_weight
    update_shipping_cost
    update_total
  end

end

Like many bad designs, this one is solved by breaking things down into smaller pieces. Here we’ve created individual methods that either update a model property or return a value. This leaves us with a single composed method, update_totals. This allows us to give intention revealing names to the component methods and explicitly define the order in which they are called. We’re alright to continue using a callback in this case because we’re only changing data within the model itself. With this approach we can still call and test each component individually.

This class could be refactored further of course. Some of these concerns probably don’t belong in the Order model at all and should be moved to service classes but we’ll leave that for another post.

External Effects

I’ve often seen callbacks used to make changes to other models or to interact with outside systems. This is generally a bad practice and is often where the callback abstraction falls apart. For example:

# app/models/comment.rb
class Comment < ActiveRecord::Base

  belongs_to :post
  belongs_to :user

  after_save :update_comment_count, :send_to_twitter, :send_email_notification

  def send_to_twitter
    TwitterWrapper.post(“Someone commented: #{self.body}”)
  end

  def send_email_notification
    CommentMailer.new_comment_email(self).deliver
  end

  def update_comment_count
    post.comment_count = Comment.where(post_id: post.id).count
    post.save
  end

end

There are two issues with this use of callbacks. First, we’ve seriously violated the single responsibility principle. If we change the way email or Twitter messages are sent in the system or even in the specific case of comments we have to reach into the model to make our change. In the case of the comment count, unbeknownst to us we have set off a chain reaction that will send a second email to the author triggered by an after_save callback in the Post model.

Beyond that, we now absolutely cannot save a comment without sending an email, posting to Twitter, and updating the count. This presents an issue in testing and in other operations that will save comments but should not have these side-effects.

In this case it’s not as easy to refactor as merely breaking things up into smaller methods, we need to separate the save action from the other behaviors. Take a look at this refactoring:

# app/models/comment.rb
class Comment < ActiveRecord::Base

    belongs_to :post
    belongs_to :user

    after_save :update_comment_count

    def update_comment_count
      post.update_comment_count!
    end

end

# app/services/creates_comments.rb
class CreatesComments

  attr_reader :post, :user

  def initialize(post, user)
    @post = post
    @user = user
  end

  def create(attrs)
    Comment.create(attrs.merge(user: user, post: post))
  end

  def create_and_notify(comment_attrs)
    comment = create(comment_attrs)
    send_to_twitter(comment)
    send_email_notification(comment)
    comment
  end

  def send_to_twitter(comment)
    TwitterWrapper.post(“#{user.display_name} commented: #{self.body}”)
  end

  def send_email_notification(comment)
    CommentMailer.new_comment_email(self).deliver
  end

end

# app/controllers/comments_controller.rb
def create
  post = Post.find(params[:id])
  creator = CreatesComments.new(post, current_user)
  creator.create_and_notify(params[:comment])
  redirect_to post_path(post), notice: “Comment added!”
end

Some may find this approach heavy-handed in this example, but the advantage here is that we’ve broken the database access concerns away from the domain behavior. When we create a new comment, we now have a simple class whose sole purpose is to deal with the side-effects of creating comments. Notice that we have left a callback in place which tells the post to update its comment count. This could also be moved into the creator class but it is something we want to always happen when adding a comment, so it may be a little safer for it to remain a callback.

This class is still talking to multiple pieces of the system and thus could be further re-factored, but I think this is an improvement. We can now call save on our comment model in tests and other scenarios without triggering the downstream actions. We can also test our CreatesComments class independently, or even call the send_to_twitter or send_email_notification methods separately if need be.

~~

In my experience callbacks can lead to a kind of soup as they tend to attract dependencies. Further, if they’re chained together across models (e.g. saving one record, saves another, saves another, sends an email) they can also lead to difficult-to-see performance issues. There are rarely absolutes when debating programming techniques however, and there is a place for callbacks. Just use them with caution and be thoughtful about their side-effects and dependencies.