ActiveRecord gives software developers a way to hook behavior into a model during persistence related activities. This can come in handy, however, everything that can be done in a callback can be done in a better, cleaner way.
class User < ActiveRecord::Base has_and_belongs_to_many :badges before_save :downcase_sensitive_fields after_create :send_welcome_email, :award_sign_up_badge after_save :award_earned_badges private def downcase_sensitive_fields self.email = email.to_s.downcase end def send_welcome_email UserMailer.welcome_email(self).deliver end def award_sign_up_badge badge = Badge.find_by_name('Sign Up') self.badges << badge end def award_earned_badges badge = Badge.find_by_name('One Year') if created_at < 1.year.ago && !badges.include?(badge) self.badges << badge end end end
We can improve this by moving each of the various callbacks to a more appropriate place and in the process simplifying our
User model. This will allow the
User model to focus on what it does best.
First, let's look at the different types of actions that are happening. Our first callback is forcing our email to lower case, which is a data manipulation action to normalize the input into something we desire. Hooks that manipulate data can be replaced with overridden setters:
class User has_and_belongs_to_many :badges def email=(val) write_attribute(:email, val.to_s.downcase) end end
Next, we see the other callbacks are things we want to happen either when a
User is created or when it is saved. Lumping these actions into callbacks makes testing harder, as you often get unwanted side effects in your tests. As we see with the badges, it also means we have a strong reliance on
Badge. You see this type of callback most often when someone has diligently followed the Skinny Controller/Fat Model paradigm. They know this code shouldn't be in the controller, so they put it in the model and since it is related to saving the trend is to use callbacks.
We can improve this situation by creating a new class that will be in charge of knowing how to handle persistence related side effects. We'll call it a
class UserService attr_accessor :user, :mail_service, :badge_service def initialize(opts) @user = opts[:user] || User.new @mail_service = opts[:mail_service] || UserMailer @badge_service = opts[:badge_service] || BadgeService.new end def create(params) user.attributes = params user.valid? && create_user end def update(params) user.attributes = params user.valid? && update_user end private def create_user if (success = user.save) mail_service.welcome_email(user).deliver badge_service.award_badges(user, new: true) end success end def update_user if (success = user.save) badge_service.award_badges(user) end success end end class BadgeService def award_badges(user, opts) sign_up_badge(user, opts) one_year_badge(user, opts) end private def sign_up_badge(user, opts) if opts[:new] badge = Badge.fetch(:sign_up) user.badges << badge unless user.badges.include?(badge) end end def one_year_badge(user, opts) if user.created_at < 1.year.ago badge = Badge.fetch(:one_year) user.badges << badge unless user.badges.include?(badge) end end end
This also gives us an opportunity to easily change how badges are awarded. We can see that there might be further areas we could improve. For example, we can shift the rules for badges to a
BadgeRule class or into the
Badge itself instead of the
BadgeService knowing the logic for each badge. Since we are still directly using an
ActionMailer for the welcome email it makes stubbing the mail_service clumsier, so we may want to further abstract that as well.
Now, our set of classes are much easier to test and it is clear what each class is responsible for. All of the business logic of what needs to happen when a user is created and saved can now be tested without a real user, real mailer, or real badges. This means no database hits and much faster tests.