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. (Read the second part here.)
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.
DevMynd – custom software development services with practice areas in digital strategy, human-centered design, UI/UX, and mobile and web application development.
