This week in refactoring, we will make a small improvement to ActiveRecord. Along the way, we will encounter and solve the following issues:

• methods hung from the wrong object
• Law of Demeter violations
• misleading comments

This refactoring began when Jason and I ran into a problem trying to alias an accessor method in Rails. This doesn't work:

  class Topic < ActiveRecord::Base
    alias_method :body, :content
  end

The problem is that ActiveRecord does not create read accessor methods until one of them is called. So even though content will be available for instances, it is not available to be aliased at class definition time. We can work around this by explicitly telling ActiveRecord to instantiate the accessor methods, like this:

  class Topic < ActiveRecord::Base
    define_read_methods
    alias_method :body, :content
  end

Unfortunately, define_read_methods is hung from the wrong object. It is an instance method, but it affects all instances of the class, and should be a class method. Let's write a unit test that anticipates the refactoring we hope to make:

  # To call alias_method, the old_name method must already exist.
  # ActiveRecord @read_methods do not exist until you call 
  # define_read_methods, which should be a class method.
  def test_define_read_method_callable_before_alias_method_during_class_definition
    assert_nothing_raised do
      c = Class.new(ActiveRecord::Base) do
        set_table_name :topics
        define_read_methods
        alias_method :body, :content
      end
    end
  end

Here's the original implementation of define_read_methods:

  def define_read_methods
    self.class.columns_hash.each do |name, column|
      unless respond_to_without_attributes?(name)
        if self.class.serialized_attributes[name]
          define_read_method_for_serialized_attribute(name)
        else
          define_read_method(name.to_sym, name, column)
        end
      end
      unless respond_to_without_attributes?("#{name}?")
        define_question_method(name)
      end
    end
  end

The two calls to self.class violate the Law of Demeter and reinforce my instinct that this should be a class method. What about the calls to define_question_method, define_read_method, and define_read_method_for_serialized_attribute? It turns out that all of these methods should be class methods. This often happens: One method hanging from the wrong object leads to a bunch of other methods in the wrong place as well.

The call to respond_to_without_attributes? is a little more trouble. This method is used to see if readers have already been defined, so that we don't define the readers over and over again. But look what the comment suggests:

  # For checking respond_to? without searching the attributes (which is faster).
  alias_method :respond_to_without_attributes?, :respond_to?

The comment is dangerous. In our case respond_to_without_attributes? is not called because it is faster, but because it is semantically necessary. The slower respond_to? would give the wrong results. I rarely add or change comments, but I am proposing this change:

  # For checking respond_to? without searching the attributes
  # Faster and sometimes required for correct semantics, e.g.
  # checking to see if attribute readers have been added for this class yet

Since we are planning to refactor all these methods to hang from the ActiveRecord::Base class, we will need some kind of instances_respond_to?, which Ruby does not provide. Here is a quick and dirty approach, marked TODO for more refactoring later:

  # TODO: Rather than creating an instance to test this, could
  # just reflect against #instance_methods. Written this way
  # to minimize the change needed to move define_read_methods
  # from instance to class method.
  def self.instances_respond_to_without_attributes?(*args)
    self.new.respond_to_without_attributes?(*args)
  end

Now we have a class-level define_read_methods. Of course, there are places in Rails that want this to be an instance method, so we should let instances delegate to classes:

  delegate :define_read_methods, :define_read_method, :to => 'self.class'

Rails' delegate is a wonderful, underused helper. It greatly reduces the cost of obeying the Law of Demeter; I wish every platform I used had an equivalent.

With that we have all the pieces. All that is left to do is create a diff, and submit the patch back to the Rails team. You can see the complete patch at Rails Trac Ticket 9109.