Breaking the Law of Demeter is Like Looking for a Needle in the Haystack
July 21st, 2008 | Published in Google Testing
by Miško Hevery
Every time I see Law of Demeter violation I imagine a haystack where the code is desperately trying to locate the needle.
The Mechanic does not care for the Context. You can tell because Mechanic does not store the reference to Context. Instead the Mechanic traverses the Context and looks for what it really needs, the Engine. So what is wrong with code like this you say? The problems with such code are very subtle:
But here is the real killer! Writing tests for such code base sucks!
Please stop looking for the needle in the haystack and just ask for the things you directly need in your code. You will thank me later...
PS: Now imagine how hard will it be to write a test for this class:
GOOD LUCK!
Every time I see Law of Demeter violation I imagine a haystack where the code is desperately trying to locate the needle.
class Mechanic {
Engine engine;
Mechanic(Context context) {
this.engine = context.getEngine();
}
}
The Mechanic does not care for the Context. You can tell because Mechanic does not store the reference to Context. Instead the Mechanic traverses the Context and looks for what it really needs, the Engine. So what is wrong with code like this you say? The problems with such code are very subtle:
- Most applications tend to have some sort of Context object which is the kitchen sink and which can get you just about any other object in the system aka the service locator.
- If you want to reuse this code at a different project, the compiler will not only need Mechanic and Engine but also the Context. But Context is the kitchen sink of your application. It tends to have reference to just about every other class in your system, hence the compiler will need those classes too. This kind of code is not very reusable.
- Even if you don't plan to reuse the code, the Context has high coupling with the rest of the system. Coupling is transitive, this means Mechanic inherits all of the badness through association.
- Your JavaDoc is not very useful! Yes, by examining the API I can see that the Mechanic needs Context, but Context is the kitchen sink. What does the mechanic really need? (If you don't have source code nearby than it may be hard to figure out).
But here is the real killer! Writing tests for such code base sucks!
- Every time I have to write a test I have to create a graph of objects (the haystack) which no one really needs or cares for, and into this haystack I have to carefully place the needles (the real object of interests) so that the code under test can go and look for them. I have to create the Context just so when I construct the Mechanic it can reach in the Context and get what it realy needs, the Engine. But context is never something which is easy to create. Since context is a kitchen sink which knows just about everything else, its constructor is usually scary. So most of the time I can't even instantiate Mechanic, not to mention run any tests on it. The testing game is over before it even started.
- Ok, but today we have fancy tools such as JMock and EasyMock, surely i can mock out Context. Yes, you can! BUT: (1) typical setup of a mock is about 5 lines of code. So your test will contain a lot of junk which will mask the real purpose of the test. (2) These tests will be fragile. Every time you refactor something in context, or how context interacts, you are running the risk of breaking your tests. (False negatives) (3) What if you want to test class Shop which needs a reference to Mechanic? Well then you have to mock out Context again. This mocking tax will be spread all over your tests. In the end the mocking setup will drown out your tests and make for one unreadable test base.
Please stop looking for the needle in the haystack and just ask for the things you directly need in your code. You will thank me later...
class Mechanic {
Engine engine;
Mechanic(Engine engine) {
this.engine = engine;
}
}
PS: Now imagine how hard will it be to write a test for this class:
class Monitor {
SparkPlug sparkPlug;
Monitor(Context context) {
this.sparkPlug = context.
getCar().getEngine().
getPiston().getSparkPlug();
}
}
GOOD LUCK!