Learning from a dropped refactoring

You don’t have to deploy every bit of code you write. In fact, it’s pretty healthy if you throw some of the code you write before you even think about committing it.

Case in point: I sat down to refactor some Sifter code this weekend. I wanted to experiment with an idea about how Rails apps should be organized. I moved some code around, stepped back, and decided the idea behind my refactoring wasn’t sufficiently well cooked and stashed it instead of committing it.

I think that one can follow SOLID principles without radically decoupling your app from Rails and especially ActiveRecord. Instead, you let AR objects do what they’re good at: representing data. You push behavior, as much as possible, out into objects. What you should end up with is a handful of models that encapsulate the tables in your database and a bunch of classes that encapsulate the logic and behavior of your application; whether these classes live in app/models or somewhere else is a matter of personal taste.

My goal is to extract an object that encapsulates user authentication behavior; a class that is mostly concerned with conditions and telling other objects to do things. I extracted a class focused on “remember me” session behavior. It looked something like this:

class UserAuthentication < Struct.new(:user)

  def remember_token?
    user.remember_expires_at.present? && (Time.now.utc < user.remember_expires_at)
  end

  def remember_me(offset=10.years)
    time = offset.from_now.utc
    user.touch_remember_token(time) unless remember_token?
  end

  def forget_me
    user.clear_remember_token!
  end
  
end

Then, for compatibility, I added this to the model:

  def authentication
    UserAuthentication.new(self)
  end

  delegate :remember_token?, :remember_me, :forget_me, to: :authentication

The principles I’m following are thus:

• Don’t expose AR’s API to collaborators. Therefore, UserAuthentication must call methods on User rather than directly update attributes and save records.
• Encapsulate behavior in non-model classes. Therefore, User shouldn’t know when or how to manipulate “remember me” data, only expose an API that handles the mechanics.

The result: now my extracted class, UserAuthentication has meaning, but doesn’t really own any behavior. It’s a bit envious of the User model. That “envy” hints that its behavior really should live in the model.

Further, using delegate means the User model’s API surface area isn’t actually reduced. Granted, this is a stopgap measure. I should really hunt down all invocations of the delegated method and convert them to use UserAuthentication instead.

At this point, it feels like my refactoring isn’t making forward progress. I’ve rearranged things a bit, but I don’t think I’ve made the code that much better.

As I alluded earlier, I decided to stash this refactoring for later contemplation. Maybe next time I should start from the callers of the APIs I want to refactor and drive the changes from there. Perhaps my conjecture about decoupling ActiveRecord data from application behavior needs tuning.

I beat the “ship lots of stuff!” drum a lot, but I’m happy with this result. Learning to strike the balance between shipping the first thing you type out and the thing your future self won’t hate is an undervalued skill. It takes practice, and that’s just what I did here. Plus, I parleyed it into a blog post. Everyone wins!