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!

4 thoughts on “Learning from a dropped refactoring

  1. Just like anything else: trade-offs, trade-offs, trade-offs!

    Instead of instantiating a UserAuthentication object within the User class, why not just put those three methods in a module and include it in User? It doesn’t completely solve your problem, but it at least makes it possible to add more authentication methods later without having to modify User.

    I would turn UserAuthentication into a decorator for User by adding #respond_to? and #method_missing and then calling UserAuthentication.new(user).remember_token?, but that would violate your first principle.

    If you really want to stick to those two principles, perhaps consider injecting the UserAuthentication role into the the user when you need to authenticate and then removing it afterwards.

    A quick search returns this like: http://rebo.ruhoh.com/dci-role-injection-in-ruby-using-method-aliasing/

    Essentially, you would be able to do this:


    You’re trading one line of code for three, but you’re also sticking to your principles. You may be able to shorten it by adding a method that assigns and removes the role around a block:

    user.with_temporary_role(UserAuthentication) do |u|

    Just an idea. I’m new to the whole DCI thing and I’m still not sure that it isn’t more complex than it’s worth!

  2. Actually, with instance_eval, you can clean it up to this:

    user.with_role(UserAuthentication) do

    # in User
    def with_role(role, &block)
    # code to add role
    instance_eval block
    # code to remove role

  3. Personally, I haven’t found any good use cases for DCI. Including a module defeats the goal of keeping the User API small.

    My original point stands: sometimes a refactoring ends us shuffling things around instead of really improving the software. That’s a hint indicating the refactoring isn’t any good and isn’t worth more effort.

  4. You’re probably right in this case. We always hear about refactoring greatly cleaning up the code, but we don’t usually hear about times when it doesn’t. Thanks for sharing!

Comments are closed.