Optional isPresent() Is Bad for You

Before Java 8, if we wanted to check whether something is null, we had to write crappy null checks. Today, we have better alternatives.

Consider a situation when we have a method that returns a logo from Person. Unfortunately, we don’t know if our person exists (in this situation, we want the default logo), so we have to check if Person is not null:

public Logo getLogoOrReturnDefault(Person person) {
    if(person != null) {
        return person.getLogo();
    }
    return new DefaultLogo();
}

Okay, we did a check, and we are happy that our code is not failing because of NPE. We can also bypass this by using a list of Persons instead of single object and do a foreach. We can also check if the list is empty before doing anything. Another solution is that we can refactor our code to use the NullObject pattern. Finally, we can stick to the “fail fast” rule — no checks and NPE when Person is null. BUT we have Java 8 in our project, so there is a special thing called Optional. Okay, wonderful, we take that object and refactor our code to Java 8 standards:

public Logo getLogoOrReturnDefault(Person person) {
    Optional<Person> optionalPerson = Optional.ofNullable(person);
    if(optionalPerson.isPresent()) {
        return optionalPerson.get().getLogo();
    }
    return new DefaultLogo();
}

Nice, no null in our code. It is better than before? No. It is easier to read? Not really. Can we do it better? Yes!
I have seen this wannabe-refactor many times. Before we create a better solution, I want to share another example of “nice” usage of Optional.

final Optional<Person> bigPerson = members.stream()
    .filter(member -> member.getLevel() > 10)
    .findFirst();
if (bigPerson.isPresent()) {
    //many lines of code here
    return something;
} else {
    return another;
}

Of course, I know why someone wrote this ugly creature. They simply do not know how Optional objects are powerful and how to use that to improve the readability of code. In my opinion, Optional's best property is not hiding nulls or telling that our method can return something or not. The biggest feature that Optional brings us is transforming. We do not simply check if an Optional object is empty or not. We can transform it to what we want (as many times as we want) in a few lines and provide alternative solutions when one of the steps of transforming is not returning existing results. So, instead of checking wheter our person exists with isPresent(), we can do this:

public Logo getLogoOrReturnDefault(Person person) {
    return Optional.ofNullable(person)
        .map(p -> p.getLogo())
        .orElse(new DefaultLogo());
}

And a second example:

final Optional<Person> highLevelPerson = members.stream()
    .filter(member -> member.getLevel() > 10)
    .findFirst()
    .map(member -> privateMethodFromManyLinesOfCode(member))
    .orElse(another);

This is a very simple example, but we can do more map operations or add many filter operations. We can do whatever we want without a single if statement or null. I suppose that all if statements can be refactored to Optional object with some operations on it. Am I wrong?

Remember one thing — if you're using Optional and you wrote isPresent() or get() you are wrong and can do it better.