Monday, July 21, 2008

Breaking the Law of Demeter is Like Looking for a Needle in the Haystack

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;
}
}

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!

9 comments:

  1. I was thinking about using Google Guice Providers to solve the Problem.

    Something 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

    ReplyDelete
  2. 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
  3. @stephan

    Yes! 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.

    ReplyDelete
  4. @apexrally Yes, it does, although they wouldn't particularly help in this problem

    ReplyDelete
  5. FYI 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
  6. Why not just push the dev team for a better design?! :)

    ReplyDelete
  7. Nice 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.

    ReplyDelete

The comments you read and contribute here belong only to the person who posted them. We reserve the right to remove off-topic comments.