Flaw: Digging into Collaborators

Avoid “holder”, “context”, and “kitchen sink” objects (these take all sorts of other objects and are a grab bag of collaborators). Pass in the specific object you need as a parameter, instead of a holder of that object. Avoid reaching into one object, to get another, etc. Do not look for the object of interest by walking the object graph (which needlessly makes us intimate with all of the intermediary objects. The chain does not need to be deep. If you count more than one period in the chain, you’re looking right at an example of this flaw. Inject directly the object(s) that you need, rather than walking around and finding them.

Warning Signs

Objects are passed in but never used directly (only used to get access to other objects)

Law of Demeter violation: method call chain walks an object graph with more than one dot (.)

Why this is a Flaw

Deceitful API

If your API says that all you need is a credit card number as a String, but then the code secretly goes off and dig around to find a CardProcessor, or a PaymentGateway, the real dependencies are not clear. Declare your dependencies in your API (the method signature, or object’s constructor) the collaborators which you really need.

Makes for Brittle Code

Imagine when something needs to change, and you have all these “Middle Men” that are used to dig around in and get the objects you need. Many of them will need to be modified to accommodate new interactions. Instead, try to get the most specific object that you need, where you need it. (In the process you may discover a class needs to be split up, because it has more than one responsibility. Don’t be afraid; strive for the Single Responsibility Principle). Also, when digging around to find the object you need, you are intimate with the intermediaries, and they are now needed as dependencies for compilation.

Bloats your Code and Complicates what’s Really Happening

With all these intermediary steps to dig around and find what you want; your code is more confusing. And it’s longer than it needs to be.
Hard for Testing
If you have to test a method that takes a context object, when you exercise that method it’s hard to guess what is pulled out of the context, and what isn’t cared about. Typically this means we pass in an empty context, get some null pointers, then set some state in the context and try again. We repeat until all the null pointers go away. See also Breaking the Law of Demeter is Like Looking for a Needle in the Haystack.

Recognizing the Flaw

This is also known as a “Train Wreck” or a “Law of Demeter” violation (See Wikipedia on the Law of Demeter).

Symptom: having to create mocks that return mocks in tests

Symptom: reading an object named “context”

Symptom: seeing more than one period “.” in a method chaining where the methods are getters

Symptom: difficult to write tests, due to complex fixture setup

Example violations: getUserManager().getUser(123).getProfile().isAdmin() // this is egregiously bad (all you need to know if the user is an admin)

context.getCommonDataStore().find(1234) // this is bad

Fixing the Flaw

Instead of looking for things, simply ask for the objects you need in the constructor or method parameters. By asking for your dependencies in constructor you move the responsibility of object finding to the factory which created the class, typically a factory or GUICE.

Only talk to your immediate friends.

Inject (pass in) the more specific object that you really need.

Leave the object location and configuration responsibility to the caller ie the factory or GUICE.

Concrete Code Examples Before and After

Fundamentally, “Digging into Collaborators” is whenever you don’t have the actual object you need, and you need to dig around to get it. Whenever code starts going around asking for other objects, that is a clue that it is going to be hard to test. Instead of asking other objects to get your collaborators, declare collaborators as dependencies in the parameters to the method or constructor. (Don’t look for things; Ask for things!)

This example mixes object lookup with calculation. The core responsibility is to multiply an amount by a tax rate.

Flaw: To test this class you need to instantiate a User and an Invoice and populate them with a Zip and an amount. This is an extra burden to testing.

Flaw: For users of the method, it is unclear that all that is needed is an Address and an Invoice. (The API lies to you).

Flaw: From code reuse point of view, if you wanted to use this class on another project you would also have to supply source code to unrelated classes such as Invoice, and User. (Which in turn may pull in more dependencies)

The solution is to declare the specific objects needed for the interaction through the method signature, and nothing more.

The most common Law of Demeter violations have many chained calls, however this example shows that you can violate it with a single chain. Getting the Authenticator from the RPCClient is a violation, because the RPCClient is not used elsewhere, and is only used to get the Authenticator.

Flaw: Nobody actually cares about the RPCCllient in this class. Why are we passing it in?

Flaw: Nobody actually cares about the HttpRequest in this class. Why are we passing it in?

Flaw: The cookie is what we need, but we must dig into the request to get it. For testing, instantiating an HttpRequest is not a trivial matter.

Flaw: The Authenticator is the real object of interest, but we have to dig into the RPCClient to get the Authenticator.

For testing the original bad code we had to mock out the RPCClient and HttpRequest. Also the test is very intimate with the implementation since we have to mock out the object graph traversal. In the fixed code we didn’t have to mock any graph traversal. This is easier, and helps our code be less brittle. (Even if we chose to mock the Authenticator in the “after” version, it is easier, and produces a more loosely coupled design).

Problem: Law of Demeter Violated to Inappropriately make a Service Locator

We need our objects to have one responsibility, and that is not to act as a Service Locator for other objects. When using Guice, you will be able to remove any existing Service Locators. Law of Demeter violations occur when one method acts as a locator in addition to its primary responsibility.

Flaw: db.getLock() is outside the single responsibility of the Database. It also violates the law of demeter by requiring us to call db.getLock().acquire() and db.getLock().release() to use the lock.

Flaw: When testing the UpdateBug class, you will have to mock out the Database‘s getLock method.

Flaw: The Database is acting as a database, as well as a service locator (helping others to find a lock). It has an identity crisis. Combining Law of Demeter violations with acting like a Service Locator is worse than either problem individually. The point of the Database is not to distribute references to other services, but to save entities into a persistent store.

The Database’s getLock() method should be eliminated. Even if Database needs to have a reference to a lock, it is a better if Database does not share it with others. You should never have to mock out a setter or getter.

Two solutions are shown: one using State Based Testing, the other with Behavior Based Testing. The first style asserts against the state of objects after work is performed on them. It is not coupled to the implementation, just that the result is in the state as expected. The second style uses mock objects to assert about the internal behavior of the System Under Test (SUT). Both styles are valid, although different people have strong opinions about one or the other.

Problem: Object Called “Context” is a Great Big Hint to look for a Violation

// The new design is simpler and will easily evolve.
public void testWithHonestApiDeclaringWhatItNeeds() {
MembershipPlan plan = new MembershipPlan();
User user = new User("Kim");
PlanLevel level = new PlanLevel(143, "yearly");
Order order = new Order("SuperDeluxe", 100, true);
plan.processOrder(user, level, order);
// Then make assertions against the user, etc ...
}
}

Context objects may sound good in theory (no need to change signatures to change dependencies) but they are very hard to test.

Flaw: Your API says all you need to test this method is a userContext map. But as a writer of the test, you have no idea what that actually is! Generally this means you write a test passing in null, or an empty map, and watch it fail, then progressively stuff things into the map until it will pass.

Flaw: Some may claim the API is “flexible” (in that you can add any parameters without changing the signatures), but really it is brittle because you cannot use refactoring tools; users don’t know what parameters are really needed. It is not possible to determine what its collaborators are just by examining the API. This makes it hard for new people on the project to understand the behavior and purpose of the class. We say that API lies about its dependencies.

The way to fix code using context objects is to replace them with the specific objects that are needed. This will expose true dependencies, and may help you discover how to decompose objects further to make an even better design.

When This is not a Flaw:

Caveat (Not a Problem): Domain Specific Languages can violate the Law of Demeter for Ease of Configuration

Breaking the Law of Demeter may be acceptable if working in some Domain Specific Languages. They violate the Law of Demeter for ease of configuration. This is not a problem because it is building up a value object, in a fluent, easily understandable way. An example is in Guice modules, building up the bindings.

// A DSL may be an acceptable violation.
// i.e. in a GUICE Module's configure method
bind(Some.class)
.annotatedWith(Annotation.class)
.to(SomeImplementaion.class)
.in(SomeScope.class);

Great post, Misko. It helps a lot improving the code design by showing the problems of not passing explicitly the objects to methods or constructors. Just one remark: In the “Problem: Law of Demeter Violated to Inappropriately make a Service Locator” section, there’s no assignment to the Lock member variable at the UpdateBug constructor.

I wouldn’t worry about objects passed in but not used directly (well, depending on the use case). Take a look at your first concrete example. The Law of Demeter says use no more than one “dot,” so for example: