When an application grows organically it accrues features like a snowball rolling down a dirty hill. With a little careful engineering, this works great (since the snowball is actually quite willing to be guided) but sometimes you need to stop and rethink things, do a little refactoring. We’ve done this (many times) with all of our products.

Most recently, I’ve been working on some cleanups and optimizations to the person avatar and company logo code in Basecamp. The evolutionary snowball (for the avatar feature, at least) progressed something like this:

  • In the beginning, there were no people avatars.
  • Then one day, we added the feature. This introduced methods on the Person model like avatar? and avatar_path and attach_avatar. There were something like seven new methods that were added to Person as a result of this.
  • Some time later, David went and cleaned it up, moving those new methods to their own module. This was great, as it kept the definition of the Person model clean, and kept the avatar-related code separate.

This brings us to today. The code remains really clean, overall, because we’ve continued to follow the pattern of moving related methods into modules, and just including those modules in the base model.

However, there are two issues with this approach. First, when a class includes lots of modules (as some of ours do), it becomes hard to keep track of where different methods are defined, and even which methods are defined. This can lead to confusion when trying to find the definition of a method, and (worst case) can allow two or more methods with the same name, which is bad. There is no warning if you have methods with the same name in multiple modules, so it isn’t hard to wind up with bugs where you’ve clobbered a method with another. You wind up double-namespacing your methods (e.g., method names in the Avatar module being prefixed with avatar_), to avoid collisions.

(Don’t misunderstand me, though. Modules are awesome. We use them a bunch, and love them. It just helps to be aware of their weaknesses, too.)

There are more issues with the module approach. What if you have different kinds of avatars (e.g., person versus group, or company)? You can’t subclass modules in Ruby, so you wind up finding other ways to solve it (like including modules in modules, etc.). Also, you can’t reference the avatar as a separate entity; instead, controllers that deal with avatars need to deal with the entity to which the avatar is attached.

Can anything be done about these issues? The avatar is a part of the Person model itself, after all, with attributes in the people table that define it. Can the avatar code be split out further, somehow?

Yes, it can. We’ll do this by making avatars a model of their own and using aggregation (rather than module inclusion, or inheritance) to pull it into Person. For example:

class Avatar
  attr_reader :master

  def initialize(master)
    @master = master
  end

  def exists?
    master.has_avatar?
  end

  def create(file)
    master.update_attribute :has_avatar, true
    # thumbnail and install the given avatar
  end

  def destroy
    master.update_attribute :has_avatar, false
    # delete avatar file
  end

  # etc.
end

class Person < ActiveRecord::Base
  # ...

  def avatar
    @avatar ||= Avatar.new(self)
  end
end

What are the benefits of this? Here are a few that occur to me:

  • We can say things like “person.avatar.exists?” and “person.avatar.create(io)”.
  • The Avatar class can be subclassed and extended, to support different avatar types.
  • The Avatar class can be tested independently of Person.
  • We don’t need to worry about methods on the Avatar model shadowing methods on the Person model.
  • It’s much more obvious where to look for the definition of “destroy” in “person.avatar.destroy”.

Also, we can now pass the avatar itself around, separate from the person:

class AvatarsController < ApplicationController
  before_filter :find_avatar

  def create
    @avatar.create(params[:avatar])
  end

  def destroy
    @avatar.destroy
  end

  protected

    def find_avatar
      person = Person.find(params[:id])
      @avatar = person.avatar
    end
end

I definitely am not suggesting that this pattern replace the use of modules. Modules are very handy, and as I said before, we still use them extensively. But if you find yourself bumping into some of the limitations of modules in a specific instance, extracting the module in question into a model can provide some really nice benefits.