Optional monad and the Law of Demeter in Java

user_1173755 0
Viewed 844

while I was reviewing some code, I came across this snippet.

List<User> users = /* Some code that initializes the list */;
users.stream()
     .filter(user -> user.getAddress().isPresent())
     .map(/* Some code */)
// And so on...

The call of method user.getAddress() returns an Optional<Address>. Following the famous Law of Demeter (LoD), the above code is not clean. However, I cannot figure out how to refactor it to make it cleaner.

As a first attempt could be to add to the User class a method hasAddress(), but this method overcomes the need of having an Optional<Address>, IMO.

How should I refactor the above code? In this case, is it worth to satisfy the LoD?

EDIT: I missed specifying that in the map method I do not want to return the address.

option-type law-of-demeter refactoring java
5 Answers

Well, you summarized it yourself pretty well: If you wanted to couple more loosely by introducing hasAddress(), why return an Optional.

Reading into what the LoD says, it talks about having "limited" knowledge about "closely related" units. Sounds like a gray area to me, but going further it also mentions the "Only one dot" rule. Still, I would agree with the comments to your question that a null check (or isPresent()) is entirely fine (heck, a real null check technically doesn't even need a dot ;P ).

If you wanted to really encapsulate more, you could remove the getAddress() completely and instead offer:

class User {
    private Optional<Address> address;

    boolean hasAddress() {
        return address.isPresent();
    }

    // still exposes address to the consumer, guard your properties
    void ifAddressPresent(Consumer<Address> then) {
        address.ifPresent(then::accept);
    }

    // does not expose address, but caller has no info about it
    void ifAddressPresent(Runnable then) {
        address.ifPresent(address -> then.run());
    }

    // really keep everything to yourself, allowing no outside interference
    void ifAddressPresentDoSomeSpecificAction() {
        address.ifPresent(address -> {
            // do this
            // do that
        });
    }
}

But again, as the commenters pointed out: Is it worth it/necessary? All these laws/principles are rarely absolute and more guidelines than dogmas. In this case it might be about balancing LoD vs. KISS.

In the end, it is up to you to decide whether this specific example benefits from moving the functionality of your stream into the User class. Both are valid, and readability/maintainability/cleanliness depend on:

  • the specific case
  • how exposed this code is to other modules
  • the number of delegate methods you would need in the User class
  • your architecture (If you are in a UserDao class for example, do you really want to move database access your User POJO class? Isn't the DAO made for exactly this purpose? Does this qualify as "closely related" and would allow a violation of the "Only one dot" rule?)
  • ...
user_76530730

Don't know if it's cleaner (as you need to use the fact that getAddress returns an optional anyway), but in Java 9 you can do:

users.stream()
    .map(User::getAddress)
    .flatMap(Optional::stream)
    .map(/* Some code */)

or

users.stream()
    .flatMap(user -> user.getAddress().stream())
    .map(/* Some code */)
user_7505630

I believe you have already answered the question yourself.

You have two distinct use cases here:

  1. Determine if the user has an address
  2. Access the user address in a null-safe way

The first use case can be solved by adding the method hasAddress(), like you stated. The second use case can be solved using the Optional to wrap the address. There is nothing wrong with that.

user_76654350

what the LoD is asking you to think about is whether or not the value of the address needs to be given out (i.e., requested/accessed) at all. If the answer is YES - then you you need to deal with the null or empty value case; otherwise, you can think about whether the existence of an address can be requested or not.

So, if its valid for the address to be given out, then returning an Optional handles the null value case and can be used to ascertain existence. Checking an optional for 'emptiness' would seem to me to be the same as checking an integer for 0 - you are not accessing an attribute of some other object, just an attribute of an object you have been given.

If only the existence is allowed to be known then an isPresent method is probably better. Of course, if you are dealing with an SQL value, then maybe an Optional would be required to handle the SQL NULL value.

Otherwise, why is this filtering happening here?

user_72279160
Related