How much should I refactor?

The Rails community has been abuzz with object-oriented programming, SOLID principles, laws, design patterns, and other principles, practices, and patterns. We’ve (re)discovered new tools and techniques to separate and reuse logic, making code easier to test, understand, and maintain. Now that we’ve learned about all these new tools, when do we use them?

Going too far

Applying design practices correctly takes a lot of practice, and when you first discover a hammer, everything looks like a nail. You can refactor every class ad-nauseum until each class obeys every letter of the SOLID principles, but that approach will hurt you: for one, not every class will ever see too much use or change, but for another, many of the abstractions advocated by those principles can hurt overall readability in the short term. Extracting a method means you need to jump around a file to figure out what’s going on. Extracting a class means you have to jump between files. Extracting a library means you need to jump between projects. Adding names can clarify what you’re doing, but adding too many names results in a vocabulary overload. Abstractions can grant you infinite flexibility but zero readability.

Getting a sour taste

If you’ve seen these principles applied incorrectly or overapplied, you may be tempted to throw the baby out with the bath water. Many Ruby developers came from Java, and after a few years of working with AbstractUserDecoratorFactoryFactories, some of them decided to never use the words “design pattern” again. Many principles have been applied with good intention and terrible results, but that’s no reason to throw out everything we’ve learned since the inception of object-oriented programming.

Applying the Dependency Inversion Principle in one situation may create an unreadable, abstract puzzle, but using it when you need it can keep a single class from ruining an entire application. Applying any principle as a black and white law will result in a game of refactoring whack-a-mole. However, if we can’t apply these principles universally, how do we know when to be aggressive about refactoring?

God classes

Every project has them: one or two classes that seem to know everything. Any question you could ask in the domain is answered by one of these classes. Any class you look into seems to depend on them. You can’t change any class in the system without breaking the tests for a god class. My experience has taught me that most projects will have two god classes: User and whatever the focus happens to be for that application. In a blog application, it will be User and Post. In an Agile project management tool, it will be User and Story. God classes grow and become entangled with every other component of the system until there’s no room to breath.

God classes are a great place to start aggressive refactoring. Without even looking inside their files, you can probably tell which two classes in your project are the culprits. I think very carefully before adding any behavior to these classes. I apply SOLID principles rigourously to these classes. If I can extract behavior from one of these classes using a design pattern like observer or decorator, I’ll do it.

Extracting behavior from a class like this doesn’t reduce overall comprehensibility, because the class is already too large to comprehend. Introducing abstractions to these classes makes them easier to understand, because you can at least fit all the abstractions in your head at once, even if you can’t remember what the implementations are like.

Churn

You can use tools like Churn to figure out which files in your project change the most. The top two contenders are likely to be your project’s god classes, but you may be surprised which other files change the most. If something changes once, it’s likely to change again, and locating the pieces of these classes that change and extracting them will make the next change faster. Refactor the classes that change the most by slimming them down and making them as readable as possible to increase productivity and help to avoid defects.

Bugs

If you can pinpoint a bug to a particular class, it’s likely that refactoring that class will help to prevent the next bug. Bugs love company, and the same parts of an application tend to break again and again. Extractions can help isolate the trickier components, and refactoring may reveal that the bugs are cropping up because you were thinking about the problem the wrong way to begin with. Keep track of which files you change when fixing bugs; these files are great targets for aggressive refactoring.

Crossing the finish line

When do you stop refactoring? When is a change good enough to commit? When is a branch good enough to deploy? When is a library good enough to release?

A program is never finished, and no amount of refactoring will make it perfect. I refactor to fix the parts of an application or library that have caused me pain. Rather than attacking theoretical problems, identify components that have actually bitten you and apply theory to tame them. Refactor as you go, and fix one problem at a time.

Rails Refactoring Example: Introduce Null Object

You probably don’t write code like this:

if object.kind_of?(User)
  do_this
else
  object.do_that
end

Why not? Because Ruby encourages duck typing and polymorphism.

A hidden version

Here’s the same principle:

if object.nil?
  do_this
else
  object.do_that
else

This checks that object is of type NilClass instead of type User.

The pattern

There’s an old pattern called Null Object that addresses this special case of avoiding type-checking in favor of duck typing.

Here’s an example of the “Introduce Null Object” refactoring to fix this problem with a Null Object in a Rails app.

The code to be refactored

Airbrake reports an error from this line in a Haml view:

= l location.orders.ascend_by_created_at.first.created_at.to_date

Demeter is displeased but let’s fix the problem for our users first.

The anti-pattern quick fix

We could do something like this:

- if location.orders.any?
  = l location.orders.ascend_by_created_at.first.created_at.to_date
- else
  No orders yet

Resist the urge.

A cleaner fix

First, re-create the error in the setup phase of a functional or integration test by creating a location without orders, then making an HTTP GET to the page.

We can change the view:

= l location.first_order_date

Much better. Bonus: we’ve given the line an intention-revealing name.

This new method doesn’t exist yet, though, so let’s test-drive it:

context 'first_order_date' do
  setup do
    @location = create(:location)
  end

  context 'when an order exists' do
    setup do
      @date = Date.today
      create :order, location: @location, created_at: @date
    end

    should 'return first order date' do
      assert_equal @date, @location.first_order_date
    end
  end

  context 'when no orders exist' do
    should 'respond to strftime' do
      assert_respond_to @location.first_order_date, :strftime
    end
  end
end

How should the duck respond?

The interesting bit is the ‘no orders’ case. Instead of returning nil, which caused the original error, we want to return an object that responds to strftime.

Why? Let’s look at the view again:

= l location.first_order_date

That l method is an alias for localize, which calls I18n.translate. If we look inside that method, we’ll see something like:

def localize(locale, object, format = :default, options = {})
  unless object.respond_to?(:strftime)
    raise ArgumentError
  end

  # ...
end

The localize method expects an object that responds to strftime.

So, to get the unit test green, let’s do this in the Location model:

def first_order_date
  if first_order
    first_order.created_at.to_date
  else
    NullDate.new 'orders'
  end
end

private

def first_order
  @first_order ||= orders.ascend_by_created_at.first
end

The Null Object

What’s this hipster NullDate object? We’ll have to write it.

The failing test for NullDate tests its strftime method:

context 'strftime' do
  setup do
    @null_date = NullDate.new('orders')
  end

  should 'return user-friendly message' do
    assert_match /No orders yet/, @null_date.strftime('anything')
  end
end

The passing implementation:

class NullDate
  def initialize(nullable)
    @nullable = nullable
  end

  def strftime(string)
    I18n.t 'models.null_date.strftime', nullable: @nullable
  end
end

A final touch

Since the intention of this feature is to display copy in the view via the localize method, we localized the response of NullDate#strftime.

In config/locales/en.yml:

en:
  models:
    null_date:
      strftime:
        No %{nullable} yet

Summary

So that works. Is it worth it? I say yes:

• It looks like a lot more code than the quick fix, but it’s quick to test-drive.
• Since the resulting Null Object is generalized, it has a high chance for re-use. In fact, it should Just Work if you drop it in your Rails app.

Next Steps & Related Reading

• Null Object in Factory Girl
• The Maybe Pattern

Written by Dan Croak.

Tidy views and beyond with Decorators

The problem

Here’s a view serving monkeys#show. What’s wrong with it?

<h1><%= @monkey.name -%>'s Monkey page</h1>
<% if @monkey.eating? %>
  <div class="eating">
    <h2><%= @monkey.name -%> is eating. Nom nom.</h2>
  </div>
<% elsif @monkey.sleeping? %>
  <div class="sleeping">
    <h2><%= @monkey.name  %> cannot be bothered right now</h2>
  </div>
<% end %>

<div id="banana-status">
<% if @monkey.bananas.any? %>
  <div class="banana-count">
    <p>The monkey has had <%= @monkey.bananas_count %> bananas so far</p>
  </div>
<% else %>
  <div class="hungry">
    <p>The monkey should really have a banana or two</p>
  </div>
<% end %>

<div id="sidebar">
  <div id="favorite-dip-sauces">
    <% if @monkey.bananas.any? %>
      <p><%= @monkey.bananas.map(&:dip_sauce).to_sentence -%></p>
    <% else %>
      <p>Eat some bananas first</p>
    <% end %>
  </div>
</div>

There’s quite a bit of logic going on here. Monkeys have different states with both DOM classes and copy changes based on it. Not only that, but banana consumption also introduces a few branches in our view code, increasing markup complexity and threatening maintainability. Now imagine this view is for a real app where it’s probably much larger and complex.

What if instead we were working with the following

<div class="<%= @monkey.state_class -%>">
  <h2><%= @monkey.status_message -%></h2>
</div>

<div id="banana-status">
  <div class="<%= @banana.status_class -%>">
    <p><%= @bananas.count_message -%></p>
  </div>
</div>

<div id="sidebar">
  <div id="favorite-dip-sauces">
    <p><%= @bananas.favorite_dip_sauces -%></p>
  </div>
</div>

Curious? Good. Here’s how you do it:

The pattern

Let’s look at how we can simplify this view by introducing a few objects that collaborate with monkeys and bananas to cleanly get at the required logic. What we need is an object that behaves just like a monkey but with some added presentation behavior: a Decorator. A decorator allows you to add, replace or extend an object’s behavior based on its runtime characteristics without resorting to any metaprogramming hacks. It is an object that wraps another object adding any required functionality, and proxies the interface you care about to original decorated object.

The refactor

We decorate a monkey object with the appropriate messages and DOM classes based on it’s state:

class MonkeysController < ApplicationController
  def show
    @monkey = decorated_monkey(Monkey.find(params[:id]))
  end

  protected
  def decorated_monkey(monkey)
    if monkey.eating?
      EatingMonkey.new(monkey)
    elsif monkey.sleeping?
      SleepingMonkey.new(monkey)
    else
      raise NoMoreMonkeysJumpingOnTheBed
    end
  end
end

The implementation for the eating and sleeping monkeys are quite simple:

class EatingMonkey
  def initialize(monkey)
    @monkey = monkey
  end

  def state_class
    "eating"
  end

  def state_message
    "#{@monkey.name} is eating. Nom nom."
  end

  def name
    @monkey.name
  end
end

class SleepingMonkey
  def initialize(monkey)
    @monkey = monkey
  end

  def state_class
    "sleeping"
  end

  def state_message
    "#{@monkey.name} cannot be bothered right now"
  end

  def name
    @monkey.name
  end
end

They are merely extending the monkey object with some view related logic. The view establishes a contract that that all monkey-like object must adhere to, but that’s it. Note that decorators also proxy message invokations to the wrapped object. In this case we’re forwarding the name method. Some folks will use method_missing to proxy all method calls over, but I’m of the mind that you should only expose the interface you care about and nothing more. This also helps document the actual contract established between the view and its collaborators.

The result is a view that is much smaller, easier to work with and extend. Not only that, but the system is now also trivial to test. Here’s an example for the state_message method on EatingMonkey

describe EatingMonkey do
  it "displays that it's eating on the state_message" do
    monkey = stub(:monkey, name: 'George')
    EatingMonkey.new(monkey).state_message.should match /George.*eating/
  end
end 

This test is great. It’s simple, verifies behavior and — because we pass in a thin test double — it informs us of the expected interface of the wrapped object: an unobstrusive thing that responds to name.

These are great improvements already, but we shouldn’t stop there. We still have some view logic around bananas that is used to control two sections of our view: the banana-status as well as the sidebar. The two cases we need to think about is when a monkey has eaten bananas, and when it hasn’t.

When it has eaten bananas, we can supply a wrapper object that helps present it. When it hasn’t, we have the special case of an empty collection. The NullObject pattern that you learned about on Josh’s post works well.

class MonkeysController < ApplicationController
  def show
    bananas = monkey.bananas
    if bananas.any?
      @bananas = Bananas.new(bananas)
    else
      @bananas = EmptyBananas.new
    end
  end
end

class Bananas
  def initialize(bananas)
    @bananas = bananas
  end

  def status_class
    "banana-count"
  end

  def count
    @bananas.size
  end

  def count_message
    "The monkey has had %{ count } bananas so far"
  end

  def favorite_dip_suaces
    @bananas.map(&:dip_sauce).to_sentence
  end
end

class EmptyBananas
  def status_class
    "hungry"
  end

  def count_message
    "The monkey should really have a banana or two"
  end

  def favorite_dip_suaces
    "Eat some bananas first"
  end
end

The OO Principles at work

We can also think of these improvements in terms of a few Object-Oriented design principles:

• The system obeys the Tell don’t Ask principle where you should tell an object to do something as opposed to extracting data from it and implementing behavior on the caller code. Here, instead of asking a monkey if it is eating in order to display a state message, we tell it to give us the message simplifying and removing branches from the client code.
• By not polluting the Monkey model, we’re also obeying the Single Responsability Principle where every object should have only one responsability and it should be encapsulated by one class. The Monkey class handles database finders, field abstraction, and data consistency via validations, not more than that. On the other hand, the decorators handle presentation related state and logic while delegating to Monkey when appropriate.
• Finally, it obeys the Open/Closed Principle where each object is open for extension but closed for modification. There is no reason to modify the Monkey class in order to add the required behavior. Instead we extend a monkey’s behavior with another set of collaborating objects.

Not only for views

Decorators are not only useful as view cleanup, although it is a very common use case. Jeff Casimir’s draper is a gem that embodies the pattern and also allows you to invoke Rails’ view helpers by exposing the view context in your decorators.

But Decorators are useful in other scenarios as well. For example, you could use them to handle all the types of notifications should occur when saving a record without resorting to a nasty callback soup:

class UserCreatedNotifier
  def initialize(saveable)
    @saveable = saveable
  end

  def save
    Mailer.user_created(@saveable).deliver
    @saveable.save
  end
end

Maybe in some cases an admin should be notified as well:

class UserCreatedAdminNotifier
  def initialize(saveable)
    @saveable = saveable
  end

  def save
    AdminMailer.user_created(@saveable).deliver
    @saveable.save
  end
end

This frees your User model from complex state or parameter checking in an already complex callback chain, and it also allows you to easily add notification mechanisms to the user creation process, or even avoid notification altogether when that’s the need:

 #notify nobody
user.save
# notify the user
UserCreatedNotifier.new(user).save
# notify the user and an admin
UserCreatedNotifier.new(UserCreatedAdminNotifier.new(user)).save
# notify the user and post to twitter
TwitterNotifier.new(UserCreatedNotifier.new(user)).save
# etc

Next Steps & Related Reading

Detect emerging problems in your codebase with. We’ll deliver solutions for fixing them, and demonstrate techniques for building a Ruby on Rails application that will be fun to work on for years to come.

Grab a free sample of Ruby Science today!

Video: Sinatra at Boston.rb, part 1

This the first in a series of short videos. They feature Blake Mizerany discussing Sinatra and Heroku in great technical detail at September’s Boston.rb.

Blake Mizerany wrote Sinatra in 2006 because he was working on a high traffic site with a lot of POSTs, PUTs, and DELETEs. GETs can be cached but the others cannot.

In this video, Blake discusses the following concepts as they apply to Sinatra:

backing into patterns

Start with a string, start with a text file, use main on Ruby. Move up to Factories and Model-View-Controller and ActiveRecord when you need it.

Rails creates 72 files when you run the rails command. Blake decided he didn’t need to start with MVC on each new project.

clean routing system

Blake uses Sinatra often for web services. He compares Rails’ respond_to with Sinatra’s content_type declaration. He discusses his feeling that the routing systems in other frameworks are overly complex undesirable.

get '/users.json' do
  content_type :json
  @user = User.find(params[:id])
  @user.to_json
end

His first example, shown above, highlights Sinatra’s clean content type approach.

next

In the next video, Blake discusses:

• handling the Accepts header using Rack middleware
• deciding between Rack and Sinatra
• rack-hoptoad
• routing tricks such as passing wildcard params into block variables and non-greedy matching