This week in refactoring, we will make a small improvement to ActiveRecord. Along the way, we will encounter and solve the following issues:
This refactoring began when Jason and I ran into a problem trying to alias an accessor method in Rails. This doesn't work:
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:
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:
Here's the original implementation of
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_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:
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:
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:
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 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.