How to solve the violations of the Law of Demeter?

问题:

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).

回答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)

回答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.Coulour

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 some 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.

回答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.

回答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 ]

回答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);
  }
}

回答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?

回答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.

回答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.

回答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:

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.

回答10:

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