Optional isPresent() is bad for you.

Before Java8 if we wanted to check if something is not 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 default logo) so we have to check if Person object 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, using list of Persons instead of single object and do foreach. We can also check if list is empty before doing any actions. Another solution is, that we can refactor our code to use NullObject pattern. Finally, we can stick to rule “fail-fast” – no checks and NPE when Person is null. BUT we have Java8 now in our project, so there is a special thing, which friends told us, called Optional. Okay, wonderful, we take that object and refactor our code to Java8 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 use that to improve readability of code. In my opinion Optional 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 Optional object is empty or not. We can transform it, to what we want (as many times as we want) in few lines and provides alternative solution when one of steps of transforming is not returning existing resul. So instead of checking if our person is exist by isPresent() we can do this:

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

or 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 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 using Optional and you wrote isPresent() or get() you are wrong and can do it better.

  • Mariusz

    “`member -> privateMethodFromManyLinesOfCode(member)“` could be simplified as “`this::privateMethodFromManyLinesOfCode“`