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.
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;
}
}
– Miško Hevery
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!
20 responses so far ↓
Yes, it is better to let the constructor receive the Engine that it needs as a parameter rather than some Context object. However, this constructor in your first example does NOT violate Law of Demeter:
Mechanic(Context context) {
this.engine = context.getEngine();
}
Considering the title of this blog posting, the reader can be mislead/confused to believe that it does, but it is indeed okay (from a LoD point of view) to invoke a method of a parameter.
Perhaps we are split on the deffinition, but in my opennion asking for an object which you don’t need and than looking for the object of interest is breaking the Law of Demeter. Asking for context falls under that deffinition.
Love it Miško – great post!
Still, when choosing between use of a context and a bunch of singletons (which is a practical choice which causes only minor refactoring), it’s a much better idea to use a context.
The other alternative is an explosion of parameters if your code deals with multiple aspects (for example: session, user, log, database, input, output)
(3) What if you want to test class Shop which needs a reference to Mechanic? Well then you have to mock out Context again.
Mock Mechanic?
But I do agree with your points and have been enjoying your writing on Singletons.
It would be funny if I was having to deal with these issues in my current assignment
When you use a factory, you usually need to call methods on the objects created by such factory. So, you aren’t you breaking the Law of Demter in that case.?
Answering Otavio…I think, the law of Demeter speaks of specifying the context/state explicitly and thus reduce the burden of learning. In most cases, things which are easier for humans are easier for machines.
However, when it comes to factories or even design patterns like state pattern, we violate this analogy. Striking the right balance between coupling and cohesion is tricky. Are we violating the LOD here?
We have to note that there is an inherent assumption made when we declare something as a Factory.( Ofcourse, this applies only for the sentient human beings). The context is made explicit in a very unorthodox way, by means of naming convention. If the object named as factory does something else, you would probably kill the developer! Death for breaking the unwritten rule.To summarise, LOD is violated in the purest sense, but for the greater good!!
To “new” or not to “new”… | Miško Hevery // Sep 30, 2008 at 9:13 pm
[...] anything wrong here? Why in the world does SongReader need to know about the MusicPlayer. This is a violation of Law of Demeter. The SongReader does not need to know about MusicPlayer. You can tell since SongReader does not [...]
As Tomas said, technically, your example does not violate the Law of Demeter because Mechanic’s constructor is referencing a method of a parameter: context.getEngine().
It would violate it if you stored a reference to context instead of the engine and later on referred tried to get the engine. For example:
class Mechanic {
Context context;
Mechanic(Context context) {
this.context = context;
}
bool CheckEngine() {
return this.context.getEngine().checkIfBusted();
}
}
However, I think an important distinction has to be made between the technical definition and the essence and intent of the LoD.
class Mechanic {
Engine engine;
Mechanic(Context context) {
this.engine = context.getEngine();
}
bool CheckEngine() {
return this.engine.checkIfBusted();
}
}
Technically, CheckEngine() might not be violating the principle, but because the explicit dependency is on Context, CheckEngine is really hiding the second-tier reference. That is: this.engine is really this.context.getEngine() even if the dot notation is obfuscating that fact.
In that sense, I think Misko is absolutely right, even if it’s more subtle than an explicit violation.
Dependency Injection Myth: Reference Passing | Miško Hevery // Oct 22, 2008 at 8:01 am
[...] the LoginPage violates this. It calls a new on User Repository. The issue here is that LoginPage is breaking a the Law of Demeter. LoginPage is asking for the Database even though it itself has no need for the Database (This [...]
Pass Around Ginormous Context Objects | Miško Hevery // Oct 27, 2008 at 2:28 pm
[...] to know it is interested in all of these!” Why is this a problem? as discussed in ‘Breaking the Law of Demeter is Like Looking for a Needle in the Haystack‘ When you ask for things explicitly it makes it easy to pass in test-doubles for these [...]
nWizard - Norbert Rakosi’s blog on Free Software development … JAVA … Personal Development… and … bad speling… » Breaking the Law of Demeter… // Nov 6, 2008 at 12:50 am
[...] Following this today I rad into an interesting article called Dependency Injection Myth: Reference Passing which lead me to another interesting reading: Breaking the Law of Demeter is Like Looking for a Needle in the Haystack. [...]
My Unified Theory of Bugs | Miško Hevery // Nov 17, 2008 at 4:36 pm
[...] separation between classes (Testable Seems) –> clear separation between classes makes it less likely that a wiring problem is [...]
I guess it would be OK to call a method of a parameter, if that is a method that actually does work. In this scenario the method is a mere getter and the context isn’t really needed, to keep its reference or to do any actual work: it is needed just to get hold of an engine! So it is clear to me that, LoD notwithstanding, Context is a bad idea in this example.
How to think about OO // Aug 7, 2009 at 8:21 am
[...] The problem here is the method may as well be static! It is in the wrong place, and you can tell this because it does not interact with any of the data in the Database, instead it interacts with the data in cache which it fetches by calling the getObjects() method. My guess is that this method belongs to one of its arguments most likely Cache. If you move it to Cache you well notice that the Cache will no longer need the getObjects() method since the for loop can access the internal state of the Cache directly. Hey, we simplified the code (moved one method, deleted one method) and we have made Demeter happy. [...]
All About Google » How to think about OO // Oct 6, 2009 at 8:37 am
[...] The problem here is the method may as well be static! It is in the wrong place, and you can tell this because it does not interact with any of the data in the Database, instead it interacts with the data in cache which it fetches by calling the getObjects() method. My guess is that this method belongs to one of its arguments most likely Cache. If you move it to Cache you well notice that the Cache will no longer need the getObjects() method since the for loop can access the internal state of the Cache directly. Hey, we simplified the code (moved one method, deleted one method) and we have made Demeter happy. [...]
Symfony versus The Law Of Demeter: does Symfony promote bad habits? // Nov 9, 2009 at 5:03 am
[...] dynamite the whole project, if they saw some typical Symfony code. One of the authors of the post, Miško Hevery, writes on his blog: Every time I see Law of Demeter violation I imagine a haystack where the code is desperately [...]
Which is better of the following two?
Mechanic(Engine e, Chassis c, Body b, Gearbox g, Paint p)
Mechanic(Context ctx)
What if Context has 9 classes and Mechanic requires 7?
@Aleemb,
Which one do you think is better? I think this one is way more truthful and gives the reader a better view to what is going on:
Mechanic(Engine e, Chassis c, Body b, Gearbox g, Paint p)
But I think you are trying to say, that there is way to much in the constructor, and I agree for you, so you should group them to logical groups and have
Mechanic(Powertrain p, Chassis c, Body b, Pain p);
But this example is a little flawed, since to make a mechanic you don’t should not need a car and paint, So I think the example you wanted to do was
Car(Powertrain p, Chassis c, Body b, Pain p);
Dependency Injection and Reference Passing « Dark Views // Jul 21, 2010 at 10:23 am
[...] Software developers are paid for their brains. If something feels wrong, it usually is. Most of the early code we come up with then starting with DI violates the Law of Demeter. [...]
Leave a Comment