Refactoring an ActiveRecord callback

Inspired by a few articles and pesentations (1 2 3 4), I decided it was time to cleanup some of the logic in my Post model related to a particular ActiveRecord callback. The fact that I needed some comments to explain what it is doing should be a red flag.

before_validation :update_published_at_if_necessary def published? self . is_published == true end def unpublished? ! published? end protected # Ensure that published_at is set accordingly. # # * Unpublished posts should not have this set # * Published posts should have it set to the current time def update_published_at_if_necessary if new_record? && published? && published_at . nil? self . published_at = Time . now end if ! published? && ! published_at . nil? self . published_at = nil end end

Fortunately, I have tests in place that excercise this logic, so as long as my tests are passing, the refactorings must work (hopefully!).

My first impression is that the callback is doing too much work. Let’s split it up into two pieces.

before_validation :set_published_at_to_now_if_necessary before_validation :unset_published_at_if_necessary protected def set_published_at_to_now_if_necessary if new_record? && published? && published_at . nil? self . published_at = Time . now end end def unset_published_at_if_necessary if ! published? && ! published_at . nil? self . published_at = nil end end

That’s somewhat better. At least the methods are more focused.

Hmm, I probably don’t need to check the existing published_at value when something isn’t published. I can also make use of unpublished?

def unset_published_at_if_necessary if unpublished? self . published_at = nil end end

I would do something similar for set_published_at_if_necessary , but I don’t want to override the published_at if it was explicitly set. Maybe I post something in the future, or past. I go a little crazy sometimes with that.

I could probably simplify the conditional logic in set_published_at_if_necessary by making a new method.

protected def set_published_at_to_now_if_necessary if new_published_post_without_published_at? self . published_at = Time . now end end def new_published_post_without_published_at? new_record? && published? && published_at . nil? end

Having if_necessary into the method names are kind of bugging me. before_filter supports :if and :unless options, so we should use those, and remove the conditionals from the callback methods.

before_validation :set_published_at_to_now , :if => :new_published_post_without_published_at? before_validation :unset_published_at , :if => :unpublished? protected def set_published_at_to_now self . published_at = Time . now end def unset_published_at self . published_at = nil end

Looking good. Looking pretty, pretty good.

Let’s see the finished product:

before_validation :set_published_at_to_now , :if => :new_published_post_without_published_at? before_validation :unset_published_at , :if => :unpublished? def published? self . is_published == true end def unpublished? ! published? end protected def set_published_at_to_now self . published_at = Time . now end def unset_published_at self . published_at = nil end def new_published_post_without_published_at? new_record? && published? && published_at . nil? end

I’m pretty happy with this. Reads really well. Some of the methods have kind of long names, but I can deal with that.