Breaking the Law of Demeter is Like Looking for a Needle in the Haystack
Monday, July 21, 2008
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.
But here is the real killer! Writing tests for such code base sucks!
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 in 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 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, it may be hard to figure out).
- 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 the 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.
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!
I was thinking about using Google Guice Providers to solve the Problem.
ReplyDeleteSomething like:
@Provider
EngineProvider(Context context) { ...}
public Engine get() { return context.getEngine(); }
Then you can have a Context and only inject Engine into the Mechanic class. The adaption is handled by Guice.
Peace
-stephan
Does Java have the concept of Generics like C#? I think a bigger question to consider is "Why is software engineering such an immature engineering discipline?"
ReplyDelete@stephan
ReplyDeleteYes! Properly using a dependency injection container is a marvelous fix to this problem. We had a similar situation in a fairly large codebase, and we slowly fixed this problem over a few months time, eventually removing almost all dependencies on Context. We used Spring, but whatever. The thing to remember is NOT to shove Context into the DI container. Just put the dependencies. We would focus on one Context "getter" at a time and slowly remove all the usages of it, eventually eliminating the getter itself.
I highly recommend doing this! Test are so much easier.
I hope the android team reads this post.
ReplyDelete@apexrally Yes, it does, although they wouldn't particularly help in this problem
ReplyDeleteFYI The needle referred to in the common phrase is not a sewing needle, it is a haystack needle, used for pinning the haystack together.
ReplyDelete:)
Is the android team reading this ?
ReplyDeleteWhy not just push the dev team for a better design?! :)
ReplyDeleteNice post, but you should be more careful about your use of "false negative". In testing, a "false negative" occurs when something was broken, yet the tests all passed. You're referring to a "false positive", in which the tests fail even though nothing is wrong.
ReplyDeletewhat if my class needs too many configs - do I need to send each as separate constructor argument or should I wrap the configs in a wrapper and send that in constructor.
ReplyDeleteBasically my class is dbutil class which needs jdbc url,user,password and table names. Now one method of dbutil fetches data from table1 and another from table2. Jdbc url,user,password,table1,table2 are read from configuration file at startup of application along with various other configs. What should I pass to dbutil constructor- a wrapper containing url,user,password,table1,table2 and store that in intance variable or just jdbc url,user,apssword in constructor and table name should be passed only when calling function of dbutil.