How to solve the violations of the Law of Demeter?

Question

A colleague and I designed a system for our customer, and in our opinion we created a nice clean design. But I'm having problems with some coupling we've introduced. I could try to create an example design which includes the same problems as our design, but if you forgive me I'll create an extract of our design to support the question.

We're developing a system for the registration of certain treatments for a patients. To avoid having a broken link to image I'll describe the conceptual UML class diagram as a c# style class definition.

class Discipline {}
class ProtocolKind 
{ 
   Discipline; 
}
class Protocol
{
   ProtocolKind;
   ProtocolMedication; //1..*
}
class ProtocolMedication
{
   Medicine;
}
class Medicine
{
   AdministrationRoute;
}
class AdministrationRoute {}

I'll try to explain a bit about the design, a protocol is the template for a new treatment. And a protocol is of a certain Kind and has medications which need to be administered. Per protocol the dosage can differ for the same medicine (amongst other things), so that's stored in the ProtocolMedication class. AdministrationRoute is the way the medicine is administered and iscreated/updated separately from the protocol management.

I've found the following places which we'll have a violation of the Law of Demeter:

Violations of the Law of Demeter

Inside of the BLL

For example, inside the business logic of the ProtocolMedication there are rules which depend on the AdministrationRoute.Soluble property of the medicine. The code would become

if (!Medicine.AdministrationRoute.Soluble)
{
   //validate constrains on fields
}

Inside the repositories

A method which would list all the protocols in a certain Discipline would be written as:

public IQueryable<Protocol> ListQueryable(Discipline discipline)
{
    return ListQueryable().Where(p => (p.Kind.Discipline.Id == discipline.Id)); // Entity Frameworks needs you to compare the Id...
}

Inside the User Interface

We use ASP.NET (no MVC) for the interface of our system, in my opinion this layer currently has the worst violations. The databinding of a gridview (a column which must display the Discipline of a protocol must bind to Kind.Discipline.Name), which are strings, so no compile time errors.

<asp:TemplateField HeaderText="Discipline" SortExpression="Kind.Discipline.Name">
   <ItemTemplate>
      <%# Eval("Kind.Discipline.Name")%>
   </ItemTemplate>
</asp:TemplateField>

So I think the actual question might be, when would it be okay to look at it more as the Suggestion of Demeter, and what can be done to solve the violations of the Law of Demeter?

I've got a few idea's of myself but I'll post them as answers so they can be commented and voted on seperatly. (I'm not sure this is the SO way to do it, if not, I'll delete my answers and add them to the question).

Scythia

My edit was rejected, but you could add this diagram. plantuml.com:80/plantuml/img/… – Charyl

Answer 1

My understanding of consequences of the Law of Demeter seems to be different to DrJokepu's - whenever I've applied it to object oriented code it's resulted in tighter encapsulation and cohesion, rather than the addition of extra getters to contract paths in procedural code.

Wikipedia has the rule as

More formally, the Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects:

1. O itself
2. M's parameters
3. any objects created/instantiated within M
4. O's direct component objects

If you have a method which takes 'kitchen' as a parameter, Demeter says you cannot inspect the components of the kitchen, not that you can only inspect the immediate components.

Writing a bunch of functions just to satisfy the Law of Demeter like this

Kitchen.GetCeilingColour()

just looks like a total waste of time for me and actually gets is my way to get things done

If a method outside of Kitchen is passed a kitchen, by strict Demeter it can't call any methods on the result of GetCeilingColour() on it either.

But either way, the point is to remove the dependency on structure rather than moving the representation of the structure from a sequence of chained methods to the name of the method. Making methods such as MoveTheLeftHindLegForward() in a Dog class doesn't do anything towards fulfilling Demeter. Instead, call dog.walk() and let the dog handle its own legs.

For example, what if the requirements change and I will need the ceiling height too?

I'd refactor the code so that you are working with room and ceilings:

interface RoomVisitor {
  void visitFloor (Floor floor) ...
  void visitCeiling (Ceiling ceiling) ...
  void visitWall (Wall wall ...
}

interface Room { accept (RoomVisitor visitor) ; }

Kitchen.accept(RoomVisitor visitor) {
   visitor.visitCeiling(this.ceiling);
   ...
}

Or you can go further and eliminate getters totally by passing the parameters of the ceiling to the visitCeiling method, but that often introduces a brittle coupling.

Applying it to the medical example, I'd expect a SolubleAdminstrationRoute to be able to validate the medicine, or at least call the medicine's validateForSolubleAdministration method if there's information encapsulated in the medicine's class which is required for the validation.

However, Demeter applies to OO systems - where data is encapsulated within the objects which operate upon the data - rather than the system you're talking about, which has different layers an data being passed between the layers in dumb, navigatable structures. I don't see that Demeter can necessarily be applied to such systems as easily as to monolithic or message based ones. (In a message based system, you can't navigate to anything which isn't in the grams of the message, so you're stuck with Demeter whether you like it or not)

Sickert

Answer 2

I know I'm going to get downvoted to total annihilation but I must say I sort of dislike Law of Demeter. Certainly, things like

dictionary["somekey"].headers[1].references[2]

are really ugly, but consider this:

Kitchen.Ceiling.Colour

I have nothing against this. Writing a bunch of functions just to satisfy the Law of Demeter like this

Kitchen.GetCeilingColour()

just looks like a total waste of time for me and actually gets is my way to get things done. For example, what if the requirements change and I will need the ceiling height too? With the Law of Demeter, I will have to write an other function in Kitchen so I can get the Ceiling height directly, and in the end I will have a bunch of tiny getter functions everywhere which is something I would consider quite a mess.

EDIT: Let me rephrase my point:

Is this level of abstracting things so important that I shall spend time on writing 3-4-5 levels of getters/setters? Does it really make maintenance easier? Does the end user gain anything? Is it worth my time? I don't think so.

Malvaceous

The main argument is that you shouldn't leak the implementation of the Kitchen throughout the rest of your application. Does the GUI really need to know how the colour of the ceiling is storred? or does the GUI just want the colour? – Scythia

@Davy: I understand the reasoning behind the Law of Demeter. Please see my edited answer. – Malvaceous

@lassevk: thanks for your insights, I responded to the first sentence "I sort of dislike". I thought I'd explain the argument for the Law of Dementer. Depending on the need of the application, I would agree with DrJokepu and use the Kitchen.Ceiling.Coulour. – Scythia

@DrJokepu: That was exactly the discussion I wanted, when is the abstraction necessary? When does it indeed make maintenance easier? In the BLL I think it's questionable if it's worth the effort. But in the GUI, where databinding parameters happen in string properties.... – Scythia

Suppose one wants to be able to use some common code to set the color of different parts of a Kitchen. If Ceiling and Floor are both derived from HorizontalSurface, which has a Colour, while Wall and Door are derived from VerticalSurface, which also has a Colour, use of common code for both may be difficult unless both derive from ColourableThing. Having Kitchen report the colourable objects it knows about, and a means of getting/setting the colour of each, would make it easier for other code to deal with new options that come available like CabinetKickplate. – Styrene

Answer 3

The traditional solution to Demeter violations is "tell, don't ask." In other words, based on your state, you should tell a managed object (any object you hold) to take some action -- and it will decide whether to do what you ask, depending on its own state.

As a simple example: my code uses a logging framework, and I tell my logger that I want to output a debug message. The logger then decides, based on its configuration (perhaps debugging isn't enabled for it) whether or not to actually send the message to its output devices. A LoD violation in this case would be for my object to ask the logger whether it's going to do anything with the message. By doing so, I've now coupled my code to knowledge of the logger's internal state (and yes, I picked this example intentionally).

However, the key point of this example is that the logger implements behavior.

Where I think the LoD breaks down is when dealing with an object that represents data, with no behavior.

In which case, IMO traversing the object graph is no different than applying an XPath expression to a DOM. And adding methods such as "isThisMedicationWarranted()" is a worse approach, because now you're distributing business rules amongst your objects, making them harder to understand.

Tirado

I agree that the LoD might not apply when a part of the design is based on data. But the reasons for LoD might still apply, certainly for the third violation? – Scythia

I don't see it on the third violation either, again because it's referencing data. This is similar to a hardcoded SQL query: there are downsides because (1) it exposes a fixed view of the data structure, and (2) it probably violates the DRY (Don't Repeat Yourself) principle because of replication. – Tirado

However, ultimately a UI does represent a fixed view of data. From a database perspective, it sometimes is a View rather than a Table, but often there's no reason to add that layer. In fact, if you've thought through the design, I'd say expose the tables first, then create views if the world changes – Tirado

+1 the distinction between classes for holding data or providing services is an important one. LoD helps you easily mock behaviour when unit testing, but there is no reason to mock pure data classes. – Insula

To use your example, would there be any violation if one asked a logger whether it was going to output a message for the purposes of avoiding going through the effort of gathering up data which the logger is simply going to discard? Or should one expect to pass the logger a callback which it should execute if it's going to decide that it actually wants the data? – Styrene

@Styrene - I think you're drawing a very fine distinction between state and intent. In my opinion, a caller that makes a decision based on the callee's intent is no different than one that examines the callee's state. Others may disagree. But I like the callback. – Tirado

I wonder if there's any term for a method/property which says whether something "might" be true, for the purpose of allowing the caller to save work in cases where it certainly isn't? Having an interface method which promises to definitively answer a question will limit the types of objects that can implement the interface, but if the the contract allows at least some "vague" answers and requires that client code be prepared to deal with them, then any type implementing the interface would be able to answer correctly if unhelpfully. To use an analogy... – Styrene

...every object in Java and .NET is required to have a hash code method such that a.equals(b) implies that a.hashCode()==b.hashCode(), but that contractual requirement doesn't impose any limitation on object design, since even if no other means of hashing were possible, an object could still satisfy the contract with int hashCode() { return 8675309; }. Thus, an assumption that two objects with different hashcodes are different does not represent any kind of assumption about the objects' ability to perform a useful hashing function. – Styrene

Answer 4

I was struggling with the LoD just like many of you were until I watched The Clean Code Talks" session called:

"Don't Look For Things"

The video helps you use Dependency Injection better, which inherently can fix the problems with LoD. By changing your design a bit, you can pass in many lower level objects or subtypes when constructing a parent object, thus preventing the parent from having to walk the dependency chain through the child objects.

In your example, you would need to pass in AdministrationRoute to the constructor of ProtocolMedication. You'd have to redesign a few things so it made sense, but that's the idea.

Having said that, being new to the LoD and no expert, I would tend to agree with you and DrJokepu. There are probably exceptions to the LoD like most rules, and it might not apply to your design.

[ Being a few years late, I know this answer probably won't help the originator, but that's not why I'm posting this ]

Autocracy

Answer 5

I'd have to assume that the business logic that requires Soluble requires other things too. If so, can some part of it be incapsulated in Medicine in a meaningful way (more meaningful than Medicine.isSoluble())?

Another possibility (probably an overkill and not complete solution at the same time) would be to present the business rule as object of its own and use double dispatch/Visitor pattern:

RuleCompilator
{
  lookAt(Protocol);
  lookAt(Medicine);
  lookAt(AdminstrationProcedure) 
}

MyComplexRuleCompilator : RuleCompilator
{
  lookaAt(Protocol)
  lookAt(AdminstrationProcedure)
}

Medicine
{
  applyRuleCompilator(RuleCompilator c) {
    c.lookAt(this);
    AdministrationProtocol.applyRuleCompilator(c);
  }
}

Flap

Answer 6

For the BLL my idea was to add a property on Medicine like this:

public Boolean IsSoluble
{
    get { return AdministrationRoute.Soluble; } 
}

Which is what I think is described in the articles about Law of Demeter. But how much will this clutter the class?

Scythia

+1 This is classic info hiding, providing a higher level service to mask the details. – Charyl

Answer 7

Concerning the first example with the "soluble" property, I have a few remarks:

1. What is "AdministrationRoute" and why would a developer expect to get a medicine's soluble property from it? The two concepts seem entirely unrelated. This means the code does not communicate very well and perhaps the decomposition of classes you already have could be improved. Changing the decomposition could lead you to see other solutions for your problems.
2. Soluble is not a direct member of medicine for a reason. If you find you have to access it directly then perhaps it should be a direct member. If there is an additional abstraction needed, then return that additional abstraction from the medicine (either directly or by proxy or façade). Anything that needs the soluble property can work on the abstraction, and you could use the same abstraction for multiple additional types, such as substrates or vitamins.

Fradin

Point 2 is a good one. But when it comes to actually implementing business logic, it is often the case that you need to consider a bunch of relevant "details" from different subobjects. The only sensible place to put that logic is in the deepest parent object that contains all relevant subobjects. – Newsmagazine

Regarding point 1, the AdministrationRoute is the way the medicine is administered. The Solubility of that route is important for certain calculations. – Scythia

Regarding point 2, The reason Soluble is not a direct member of medicine is because we'd like to reduce the redundant data. AdministrationRoute is a static-table kind of relation. These are managed separately from the protocols. – Scythia

Sounds to me like you'd want to separate the administration route and the medicine properties (in the various subtypes). Try setting up a third-normal-form database table design that describes what you want to do with the data, it may tell you how you want to decompose your classes. – Fradin

Answer 8

Instead of going all the way and providing getters/setters for every member of every contained object, one simpler alteration you can make that offers you some flexibility for future changes is to give objects methods that return their contained objects instead.

E.g. in C++:

class Medicine {
public:
    AdministrationRoute()& getAdministrationRoute() const { return _adminRoute; }

private:
    AdministrationRoute _adminRoute;
};

Then

if (Medicine.AdministrationRoute.Soluble) ...

becomes

if (Medicine.getAdministrationRoute().Soluble) ...

This gives you the flexibility to change getAdministrationRoute() in future to e.g. fetch the AdministrationRoute from a DB table on demand.

Newsmagazine

I understand what you suggest, but because I'm using c# (and EF) the relations are already properties. In c# this means automatic getters and setters. – Scythia

This does not change anything in C# since a property expose a member (and at compile time they automagically generate as get_ and set_ methods). This will also NOT solve the Law Of Demeter problem, since the calling object will still be "coupled" to the AdministrationRoute class – Invercargill

@veggerby: I don't think this answer was meant to solve the Law of Demeter problem, but more about how to add more "flexibility". Flexibility does not decrease the coupling. – Scythia

Sure but this would change nothing in C# since you can/are doing the same with properties and if you really want to add flexibility and decoupling (at the same time) you should be exposing only interfaces not implementations – Invercargill

Answer 9

I think it helps to remember the raison d'être of LoD. That is, if details change in chains of relationships, your code could break. Since the classes you have are abstractions close to the problem domain, then the relations aren't likely to change if the problem stays the same, e.g., Protocol uses Discipline to get its work done, but the abstractions are high level and not likely to change. Think of information hiding, and it's not possible for a Protocol to ignore the existence of Disciplines, right? Maybe I'm off on the domain model understanding...

This link between Protocol and Discipline is different than "implementation" details, such as order of lists, format of data structures, etc. that could change for performance reasons, for example. It's true this is a somewhat gray area.

I think that if you did a domain model, you'd see more coupling than what is in your C# class diagram. [Edit] I added what I suspect are relationships in your problem domain with dashed lines in the following diagram:

_{(source: plantuml.com)}

On the other hand, you could always refactor your code by applying the Tell, don't ask metaphor:

That is, you should endeavor to tell objects what you want them to do; do not ask them questions about their state, make a decision, and then tell them what to do.

You already refactored the first problem (BLL) with your answer. (Another way to abstract BLL further would be with a rule engine.)

To refactor the second problem (repositories), the inner code

    p.Kind.Discipline.Id == discipline.Id

could probably be replaced by some kind of .equals() call using a standard API for Collections (I'm more of a Java programmer, so I'm not sure of the precise C# equivalent). The idea is to hide the details of how to determine a match.

To refactor the third problem (inside the UI), I'm also not familiar with ASP.NET, but if there's a way to tell a Kind object to return the names of Disciplines (rather than asking it for the details as in Kind.Discipline.Name), that's the way to go to respect LoD.

Charyl

Answer 10

The third problem is very simple: Discipline.ToString() should evaluate the Name property That way you only call Kind.Discipline

Geffner

Answer 11

Pete Kirkham's analogy to "let the dog handle its own legs" is even more hilarious and better than the time-honored comparison with the Big Mac which I have so long used:

So if you have a method that takes a "BigMac" as an argument and needs to access the seeds on the pickles, you don't have to know about the lettuce, onions, and sesame seed bun that are all "on route" to the pickle seeds (much less that there are 2 all beef patties lurking underneath all of that), all you need to know is that a path exists from the BigMac to its pickle seeds. You don't have to know what that path is or exactly how to traverse it, and you don't have to encode that path in your methods. That way when the burger later changes such that the pickles are now on top of the onions instead of the other way around, you don't have to care. Brad Appleton, Introducing Demeter and its Laws

Tropine