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.
