Does this violate the Law of Demeter?

Question

Let's say I have a class SelectableEntity<T extends Entity> which has three methods, select, deselect, isSelected and count.

To take a somewhat contrived example, let's say I'm building an emergency messaging application that allows me to message to residents during fires, floods and other natural disaster.

I can select multiple entities of different types (but all for the same reason). Let's say these entities are House, Street and Suburb.

Now let's say I have a class called MessageRecipients that contains a SelectableEntity<House>, SelectableEntity<Street>, SelectableEntity<Suburb>.

There are two ways I could code this:

The first way is to create getters (i.e. houses, streets, suburbs) and select / deselect recipients that way.: messageRecipients.streets.select(aStreet).

The other way would be to hide the fact that MessageRecipients is using SelectableEntity at all and simple create 'proxy' methods for each method on SelectableEntity (i.e. selectHouse, deselectHouse, selectStreet, etc).

The first method seems like it violates the Law of Demeter whereas the second method basically requires me to create a whole bunch of methods on MessageRecipients that just directly invoke the relevant instance of SelectableEntity (and requires a whole bunch of extra testing to ensure the correct methods are being invoked).

My question is, is the first example a standard example of a violation of the Law of Demeter and would it preferable to stomach all the duplication and verbosity and go down the second approach (a guess a follow question is what constitutes a valid reason to break the Law of Demeter, if anything)?

Note: The example I've given is made-up and I'm aware that it might break down in certain situations (i.e. streets and suburbs contain houses - if I add a street which has house XYZ and I call messageRecipients.houses().isSelect(houseXYZ) it should return true). For the sake of this discussion that case should be ignored as it doesn't apply to the actual scenario I'm dealing with.

Answer 1

Neither of your proposed solutions matches the requirements you mentioned, and I suspect that is why it does not fit (it either violates LoD, or needs proxy methods for things).

From what you described, let me propose a different design. As I understand MessageRecipients are Residents. So let's forget the technical term, and stick with Residents. The point of Residents is that they can be alerted. So let's write that down:

public interface Residents {
    void alert(Message msg);
}

Now as I understand it, there are different types of "locations" which contain residents. These things basically represent some set of residents who can be alerted together. So these are a special kind of "residents" for the purposes of our application.

public final class House implements Residents {
    ...
    public House(...address, etc?...) {
        ...
    }

    @Override
    public void alert(Message msg) {
        ...
    }
}

Also, you may want to alert House, Suburb and Street residents together. In this case you need an "aggregate" implementation like this:

public final class AggregateResidents implements Residents {
    private final List<Residents> residents;

    public AggregateResidents(List<Residents> residents) {
        this.residents = residents;
    }

    @Override
    public void alert(Message msg) {
        residents.forEach(r -> r.alert(msg));
    }
}

That's it. You can plug these things together in any way you like, and can alert any combination of residents without violating LoD, or needing proxy methods anywhere.

I know, you probably have other requirements that might make this design invalid. The point I'm making is that try to get away from a technical design, away from data modeling, and try to concentrate on the non-technical concepts. That is sometimes actually hard, but it will automatically take care of all the laws and rules.

Answer 2

There are two ways I could code this:

The first way is to create getters (i.e. houses, streets, suburbs) and select / deselect recipients that way.: messageRecipients.streets.select(aStreet).

Don't think I prefer this way but I don't want to see the Law of Demeter misapplied.

Some think this is all LoD has to say:

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:

• O itself
• m's parameters
• Any objects created/instantiated within m
• O's direct component objects
• A global variable, accessible by O, in the scope of m

In particular, an object should avoid invoking methods of a member object returned by another method. For many modern object oriented languages that use a dot as field identifier, the law can be stated simply as "use only one dot". That is, the code a.b.Method() breaks the law where a.Method() does not. As an analogy, when one wants a dog to walk, one does not command the dog's legs to walk directly; instead one commands the dog which then commands its own legs.

Wikipedia: Law of Demeter

This is a monument to structural thinking. There is not a single semantic or conceptual thought in any of that. A static analyzer could enforce this. Bleh. This is not all I think of when I think of the LoD.

The Law of Demeter is not a dot counting exercise.

When you are an object, it is better to only talk to your friends, not friends of friends. If you randomly pull in anything you happen to need you'll soon find you've pulled together so many things that any change to the code base is going to hurt you right here.

Friends are things whose interfaces you own. Only then are you promised that you could jump through these dots and always find the same thing. While that can happen it's not going to be true if you just randomly grab whatever you like out of a code base.

So if the client calling "messageRecipients.streets.select(aStreet)" owns the interface for messageRecipients and streets then it's talking to its friends and there is no LoD violation. This means the client owns all the interfaces it uses. They should not change without the client's permission. This should be made clear to maintenance programmers. This needs clear boundaries. But it's not about dot counting.

That's the real problem. Because of the way you're getting this other object, the dependence on it's interface isn't explicit. The dependence can be a surprise. If you respect that you'll find some way to make this dependence clear to other coders.

This shouldn't be some cobbled together situation. This should be a situation created by careful design. What's being created here actually has a name. It's called a DSL. If that's what you're making fine. If this is just some randomly thrown together stuff because you happened to need it then you're creating a problem waiting to happen.

The other name the LoD goes by is "the principle of least knowledge". It's asking you to understand that when you create these dot chains you're adding knowledge to your client not only of these interfaces but how they connect. The more you know the more it hurts when things change. Don't create chains no one promised would be stable.

The other way would be to hide the fact that MessageRecipients is using SelectableEntity at all and simple create 'proxy' methods for each method on SelectableEntity (i.e. selectHouse, deselectHouse, selectStreet, etc).

I like this idea, but i'd also hide whether what's being selected is a house, street, or suburb. Let the suburb be the only thing that cares that it's a suburb.

Answer 3

If I understand you correctly your Selectable is a collection object with syntax for marking some of its items as selected.

In of itself this is fine, but you usage, exposing more than one collection on your MessageRecipients object, is bad(tm)

The LoD is a bit vague, but we can list the bad things about exposing the three types of list

1. Calling code has to know what type of thing it is trying to select and match to the appropriate property.

2. Additional types added later (state?) will change the signature of MessageRecipients

Your second solution, adding methods, also suffers from these problems though.

Ideally you want.

MessageRecipients.Select(IAlertable recipient)

Since the purpose of the object is to send alerts we only care that recipients can do that.

Internally maybe we know about the types and have the three lists. But because its internal we can add to them later and not affect calling code.

Nb. Depending on the detail, you might go for ISelectable or just string Id