Inspired by a few articles and pesentations (1234), 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.
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.
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?
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.
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.
Looking good. Looking pretty, pretty good.
Let’s see the finished product:
I’m pretty happy with this. Reads really well. Some of the methods have kind of long names, but I can deal with that.
The only part I don’t really like is published? and unpublished?, but that’s for another day.