Optional Anti-Pattern

It might seem obvious, but recently I saw a colleague use Optional in a weird way.

When I see a piece of code that seems to contain about 3 or more Optional.isPresent() if checks and Optional.get() method calls, I kind of panic.

To be absolutely fair, the code wasn't ready yet, so was going to change anyways.

So, anyways, before we had Optionals, we had to deal with NULL a lot.

The way to do this was as follows:

  private boolean documentAccessAllowed() {
    Information information = session.getInformation();
    if (information != null) {
      Document document = database.getDocument(information.getDocumentnumber());
      if (document != null) {
        if (!document.getOwner().equals(user)) {
          Administrator administrator = database.getAdmininstrator(user.getName());
          if (administrator == null) {
            addErrorMessage("You are not allowed access to this document.");
            return false;
          }
        }
      }
    }
    return true;
  }

I realize this code could be refactored using "Replace Nested Conditional with Guard Clauses", but some purists frown upon multiple return statements in a method. I don't mind. But let's continue with the example as is.

Then we got Optionals, yay! But, translating the above code verbatim causes my head to explode:

  private boolean documentAccessAllowed() {
    Optional<Information> information = session.getInformation();
    if (information.isPresent()) {
      Optional<Document> document = database.getDocument(information.get().getDocumentnumber());
      if (document.isPresent()) {
        if (!document.get().getOwner().equals(user)) {
          Optional<Administrator> administrator = database.getAdmininstrator(user.getName());
          if (administrator.isEmpty()) {
            addErrorMessage("You are not allowed access to this document.");
            return false;
          }
        }
      }
    }
    return true;
  }

This code is of course hardly ideal!

A better way is to use the flatMap and map and filter functions of Optional, which seems more concise, but requires a bit of a mental adjustment.

  private boolean documentAccessAllowed() {
    Boolean allowed = session.getInformation()
        .map(Information::getDocumentnumber)
        .flatMap(database::getDocument)
        .filter(doc -> !doc.getOwner().equals(user))
        .filter(doc -> database.getAdmininstrator(user.getName()).isEmpty())
        .isEmpty();
    if (!allowed) {
      addErrorMessage("You are not allowed access to this document.");
    }
    return allowed;
  }

The advice of a colleague of mine is that lambdas, even slightly trivial ones, are a bit hard to read, and making it a method with a good name helps clear things up immensely, like so:

private boolean documentAccessAllowed() {
    boolean allowed = session.getInformation()
        .map(Information::getDocumentnumber)
        .flatMap(database::getDocument)
        .filter(doc -> !isOwner(user, doc))
        .filter(doc -> !isAdministrator(database, user))
        .isEmpty();
    if (!allowed) {
      addErrorMessage("You are not allowed access to this document.");
    }
    return allowed;
  }

The idea here is not to make the code short (although that most often helps) but make the code very easy to read and follow.

In the example above, there's a lot of use of negation, which I don't like either. (isEmpty(), !isAdmin, !isOwner) Sometimes shoehorning boolean expressions so you can put them in sequential filters is a bad idea. Perhaps the following is more clear:

  private boolean documentAccessAllowed(Session session, Database database, User user) {
    boolean allowed = session.getInformation()
        .map(Information::getDocumentnumber)
        .flatMap(database::getDocument)
        .filter(doc -> isOwnerOrAdministrator(database, user, doc))
        .isPresent();
    if (!allowed) {
      addErrorMessage("You are not allowed access to this document.");
    }
    return allowed;
  }

  private static boolean isOwnerOrAdministrator(Database database, User user, Document doc) {
    return isOwner(user, doc) || isAdministrator(database, user);
  }

Let me know if you have any suggestions, of which there are no doubt several, on how this can be improved in readability.