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)

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

  def forget_me

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

  def authentication

  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!

Refactoring to more code

Refactor my code is a neat site where you can post your code and watch others refactor it. I saw an interesting bit of code whiz past and thought I’d take a crack at it.

Removing conditionals from code is one of the little games I sometimes play while coding. Here, I’ve extracted the logic of the conditional into another class. The resulting class is _much more_ code than the original. So why do that?

Well, I say you get a few benefits:

* The logic is now far easier to test. It’s a standalone object now rather than a Rails functional test.
* The flow of what’s being done and tested is more decomposed and easier to follow.
* Most importantly, the code explains itself. No need for comments (which will undoubtedly go out of sync over time) here!

While I delight in deleting code and writing as little as possible, refactoring this to _more_ code seems the right way. What say you?

Update: Make sure you check out Marcel Molina’s refactoring. Its probably better than mine ;)