Language-Agnostic, Clean Code – Is This Code a ‘Train Wreck’ Violating the Law of Demeter?

clean codelanguage-agnostic

Browsing through some code I've written, I came across the following construct which got me thinking. At a first glance, it seems clean enough. Yes, in the actual code the getLocation() method has a slightly more specific name which better describes exactly which location it gets.

service.setLocation(this.configuration.getLocation().toString());

In this case, service is an instance variable of a known type, declared within the method. this.configuration comes from being passed in to the class constructor, and is an instance of a class implementing a specific interface (which mandates a public getLocation() method). Hence, the return type of the expression this.configuration.getLocation() is known; specifically in this case, it is a java.net.URL, whereas service.setLocation() wants a String. Since the two types String and URL are not directly compatible, some sort of conversion is required to fit the square peg in the round hole.

However, according to the Law of Demeter as cited in Clean Code, a method f in class C should only call methods on C, objects created by or passed as arguments to f, and objects held in instance variables of C. Anything beyond that (the final toString() in my particular case above, unless you consider a temporary object created as a result of the method invocation itself, in which case the whole Law seems to be moot) is disallowed.

Is there a valid reasoning why a call like the above, given the constraints listed, should be discouraged or even disallowed? Or am I just being overly nitpicky?

If I were to implement a method URLToString() which simply calls toString() on a URL object (such as that returned by getLocation()) passed to it as a parameter, and returns the result, I could wrap the getLocation() call in it to achieve exactly the same result; effectively, I would just move the conversion one step outward. Would that somehow make it acceptable? (It seems to me, intuitively, that it should not make any difference either way, since all that does is move things around a little. However, going by the letter of the Law of Demeter as cited, it would be acceptable, since I would then be operating directly on a parameter to a function.)

Would it make any difference if this was about something slightly more exotic than calling toString() on a standard type?

When answering, do keep in mind that altering the behavior or API of the type that the service variable is of is not practical. Also, for the sake of argument, let's say that altering the return type of getLocation() is also impractical.

Best Answer

The problem here is the signature of setLocation. It's stringly typed.

To elaborate: Why would it expect String? A String represents any kind of textual data. It can potentially be anything but a valid location.

In fact, this poses a question: what is a location? How do I know without looking into your code? If it were a URL than I would know a lot more about what this method expects.
Maybe it would make more sense for it to be a custom class Location. Ok, I wouldn't know at first, what that is, but at some point (probably before writing this.configuration.getLocation() I would take a minute to figure out what it is this method returns).
Granted, in both cases I need to look some place else to understand, what is expected. However in the latter case, if I understand, what a Location is, I can use your API, in the former case, if I understand, what a String is (which can be expected), I still don't know what your API expects.

In the unlikely scenario, that a location is any kind of textual data I would reinterpret this to any kind of data, that has a textual representation. Given the fact, that Object has a toString method, you could go with that, although this demands quite a leap of faith from the clients of your code.

Also you should consider, that this is Java you're talking about, which has very few features by design. That's what's forcing you to actually call the toString at the end.
If you take C# for example, which is also statically typed, then you would actually be able to omit that call by defining behavior for an implicit cast.
In dynamically typed languages, such as Objective-C, you don't really need the conversion either, because as long as the value behaves like a string, everybody is happy.

One could argue, that the last call to toString is less a call, than actually just noise generated by Java's demand for explicitness. You're calling a method, that any Java object has, therefore you do not actually encode any knowledge about a "distant unit" and thereby don't violate the Principle of Least Knowledge. There is no way, no matter what getLocation returns, that it doesn't have a toString method.

But please, do not use strings, unless they are really the most natural choice (or unless you're using a language, that doesn't even have enums ... been there).

Related Solutions

Clean Code – Solving ‘Train Wreck’ Properties Violating Law of Demeter

After reading the very interesting article posted by Robert Harvey, I'd suggest that if you're still worried about the LoD you do this (forgive me for not using ObjC syntax here, I have no clue how that works):

/* Old code: */
void update() {
  this.priceLabel.text = this.media.ad.price.value;
}

/* New code: */
void update() {
  updateAdInfo(this.media.ad);
}
void updateAdInfo(Ad ad) {
  // Can dive a level deeper here if you want to.
  this.priceLabel.text = ad.price.value;
}

Rationale: By introducing additional getters for sub-properties, you're flattening out your domain model, and that's just horrible: you lose all structure, and increase coupling. (What if you add a property to Ad? Do you now go to every single class that embeds an Ad and add getters for the new property?)

However, if you have a hierarchy of objects and a matching (partial) pseudo-hierarchy of view elements to display them, it makes sense to organize your update functions in a hierarchy as well.

Design – How do we keep dependent data structures up to date

I think your scenarios are discussing variations on the Observer Pattern. Each original node (“subject”) has (at least) the following two methods:

registerObserver(observer) – adds a dependent node to the list of observers.
notifyObservers() – calls x.notify(this) on each observer

And each dependent node (“observer”) has a notify(original) method. Comparing your scenarios:

The notify method immediately rebuilds a dependent subtree.
The notify method sets a flag, the recomputation happens after each set of updates.
The notifyObservers method is smart and only notifies those observers whose constraints are invalidated. This would probably use the Visitor Pattern, so that the dependent nodes can offer a method that decides this.
(this pattern has no relation to brute-force rebuilding)

As the first three ideas are just variations on the observer pattern, their design will have similar complexity (as it happens, they are actually ordered in increasing complexity – I'd think №1 is the most simple to implement).

I can think of one enhancement: building the dependent trees lazily. Each dependent node would then have a boolean flag that is either set to valid or invalid. Each accessor method would check this flag and, if necessary, recalculate the subtree. The difference to №2 is that recalculation happens on access, not upon change. This would probably result in fewest computations, but can lead to significant difficulties if the type of a node would have to change upon access.

I would also like to challenge the need for multiple dependent trees. For example, I always structure my parsers in a way that they immediately emit an AST. Information that is only relevant during construction of this tree doesn't have to be stored in any permanent data structure. Likewise, you can also choose your objects in such a way that the AST has an interpretation as a control flow graph.

For a real-life example, the compiler part inside the perl interpreter does this: The AST is built bottom-up, during which some nodes are constant-folded away. In a second run, the nodes are connected in execution order, during which some nodes are skipped by optimizations. The result is very fast parsing (and few allocations), but very limited optimizations. It should be noted that while such a design is possible, it is probably not something you should strive for: It is a ~~calculated trade-off~~ complete violation of the Single-Responsibility Principle.

If you do actually need multiple trees, then you should also consider whether they really have to be built simultaneously. In the majority of cases, a parse tree is constant after the parse. Likewise, an AST will probably stay constant after macros are resolved and AST-level optimizations have been executed.

Best Answer

Related Solutions

Clean Code – Solving ‘Train Wreck’ Properties Violating Law of Demeter

Design – How do we keep dependent data structures up to date

Related Topic