Recently, my friend Avdi Grim posted a great explanation of the Law of Demeter (LoD). It’s been tweeted and posted all over the web, so hopefully you saw it and read it. I’m totally in agreement with his conclusions in that post, and LoD is a code smell I see in every MVC project I have ever worked in.

I’m going to pick on Avdi’s post just a little, but this is more-so criticism of every LoD explanation I’ve ever read. I think all of these explanations of LoD are actually quite contrite and not addressing the real problem. Avdi (and others) seem to focus on “Structrual Coupling” or exposing the “internal” (actually external*) structure of your objects to other objects that don’t need to know it.

An Example:

<code class="ruby">def user_info(user) "Name: #{user.name}. Boss: #{user.department.try(:head).try(:name)}" end

Avdi does a beautiful job illustrating why this code is a problem but I want to go a little deeper.

The more insidious LoD violation see in codebases is what I’d call “behavioral” LoD violations, or, when the behavior of a class relies on the behavior of objects that are distant in the object graph.

My example needs a little setup. Lets say you have Vehicles which has_many Platforms which has_many Models which has_many Versions (and Platforms has many Version through models). Pretty simple 4 level deep object graph that a car manufacturer might have. A very common set of objects you might want is to know which Platforms are actually in production. Some our outdated, some are future, but you want all the platforms that are currently in production.

Now, I know how I’d do this, but I commonly see code like this:

<code class="ruby">class Vehicles
def current_platforms(options = {})
platforms = platforms.find(:all,
:conditions =>
["platforms.on_date >= ? AND platforms.status IN ('open', 'cancelled') AND platforms.cached_inventory_status != ?",
Date.today.to_s(:db),
Platform.cached_inventory_status(:no_inventory)],
:order => 'on_date asc, time_note asc',
:include => {:models => :versions})

if options[:skip_some_special_case]
platforms.select { |platform| platform.versions.any? { |offer| !version.special_case} }
elsif options[:skip_some_other_case]
platforms.select { |platform| platform.versions.any?(&:other_case?) }
else
platforms
end
end
end

This code is a great example of my problem with how most folks end up simplifying LoD violations. No where are we chaining methods “too deep” or as it gets simplified to are we “using too many periods”. We’ve only used 1 period (because of a has_many :through =>). However, in our if statements, we are tightly coupling our code to the behavior of a class that is 3 siblings away from the one we’re working in. Nasty. Never should a Platform do operations on, or even know about, a Version.

So why is my example actually worse than something.try(:something_else).try(:a_third)? Well, lets get back to that * I left on “(actually external [structure])”.

Most people seem to classify the active_record generated attribute accessors of Rails model classes as “internal structure” (Avdi doesn’t, I should state for correctness). These are most certainly external structure. You can override them and return whatever you’d like and attributes[:foo] is actually the “internal” accessor for all active_record classes. External structure, in every project I’ve ever worked in, has been the most consistent, least volatile part of the codebases. External structure almost *never* changes, and when it does, it’s typically *very* easy to change (it’s not even a refactoring, it’s just changing the messages exchanged between classes).

My example relies on *behavior* of a class 3 classes away. If there’s anything I’ve learned, it’s that behavior is always more volatile than structure, and as Avdi points out, LoD really starts hurting as code churns and whatever your LoD relies on changes and becomes more and more complex.

So the big question is, how do we fix this?

It’s actually a theoretically simple fix, and similar to the fixes in Avdi’s post: delegate the behavior down the object graph!



class Vehicle def current_platforms(options={}) if options[:skip_some_special_case] platforms.reject(&:special_case?) elsif options[:skip_some_other_case] platforms.select(&:other_case?) else platforms.select(&:regular_case) end end end

 class Platform def regular_case ... end def special_case? models.special_case? end def other_case? models.other_case? end end class Model def special_case? versions.special_case? end def other_case? versions.other_case? end end class Version def special_case? ... end def other_case? ... end end Now, Platform knows nothing about versions, versions can easily change what defines the “cases” without any changes to Platform and ironically, when I made this refactoring to the original source I got this derived example from, it’s actually more efficient. Removing the explicit find that started the original method means that if we’ve already loaded this object graph, even outside of this method, we can keep reusing it without going to the database. I hope this helps, and for the love of Demeter, can we stop describing it as “using too many periods”?