Refactoring From Model to View Helper to Null Object

We are working to add more thorough progress tracking to upcase.com.

Chad and I were pairing on the backend for this feature, and we created a model called Status, that belongs both to a User and to an Exercise. This is a many-to-many relationship that holds a state attribute. That way we can query, for each exercise, what its state is for the currently signed in user.

When a user starts an exercise, we create a Status object, with a state of "Started". If there is no related Status, then the exercise was not yet started (and we display to the user "Not Started").

Initial Implementation: Model method

Initially we implemented the following method in the Exercise model:

class Exercise < ActiveRecord::Base
  def state_for(user)
    if status = status_for(user)
      status.state
    else
      "Not Started"
    end
  end

  def status_for(user)
    statuses.where(user: user).order(:created_at).last
  end
end

This made the tests green, and now it was time to refactor. The model was responsible of knowing what to display in the view for an exercise that was not started (the "Not Started" string). This was obvious while writing tests. We moved from integration into unit tests, and discovered we were asserting the label shown to the user inside of the model spec.

First Refactoring: View Helpers

We felt this belonged to the View layer, and moved the method into a helper:

module ExercisesHelper
  def state_for(user:, exercise:)
    if status = exercise.status_for(user)
      status.state
    else
      "Not Started"
    end
  end
end

At this point, Chad and I pushed up our latest changes and opened a pull request. The feedback we received provided us with an even better solution:

Second Refactoring: Null Objects

The helper’s conditional decides what is the state for a Status that doesn’t yet exist. The state of a NullStatus. We chose to name this null object NotStarted because it makes explicit why we need it, rather than just denoting what it is.

The third and best implementation we found is:

class NotStarted
  def state
    "Not Started"
  end
end

class Exercise < ActiveRecord::Base
  def status_for(user)
    statuses.where(user: user).order(:created_at).last || NotStarted.new
  end
end

In the view we always show state, and the corresponding object will always tell us:

<%= exercise.status_for(current_user).state %>

We Replaced Conditional with Polymorphism. No wonder the reviewer was also the author of the Polymorphism blog post, thank you Joe! Thank you twice.

What’s next

Upcase subscribers get access to the Upcase GitHub repo, where they are welcome to see all new features, improvements, and refactorings as we make them.

If you already have access, you may check the pull request with the whole discussion (PR #914). Otherwise, you can always get access to this and more thoughtbot content and repositories through Upcase.

Enjoy!