Expanding on the Law of Demeter
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:
def user_info(user) "Name: #{user.name}. Boss: #{user.department.try(:head).try(:name)}" endAvdi 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:
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
endThis 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
endNow, 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”?
