[FIXED] Optional orElse() cannot return alternative value when an Exception occurs

Issue

I am using this logic and try to return new Employee() if no employee is found.

However, when request.getId() is null, it throws exception, and I am waiting that it executes orElse() part. However, it does not. So, should I use another optional method, e.g. orElseGet() or ifPresentOrElse()?

How can I fix this problem in a smart way?

Employee employee = employeeRepository.findById(request.getId())
        .orElse(new Employee());

Update: If using Optional is not a good idea, I think of using the following approach:

Employee employee = new Employee();
if (id != null) {
    employee = employeeRepository.findById(id)
                   .orElse(new Employee());
}

Any idea for that?

Solution

Using Optional to hide a null-check - is an Antipattern

There's nothing wrong with implicit null-checks e.g. if (something == null) and more over Optional wasn't designed to perform null-checks.

Here's an excerpt from the answer by JDK developer Stuart Marks:

The primary use of Optional is as follows: (slide 36)

Optional is intended to provide a limited mechanism for library method return types where there is a clear need to represent "no result," and where using null for that is overwhelmingly likely to cause errors.

The ability to chain methods from an Optional is undoubtedly very cool, and in some cases it reduces the clutter from conditional logic. But quite often this doesn't work out. A typical code smell is, instead of the code using method chaining to handle an Optional returned from some method, it creates an Optional from something that's nullable, in order to chain methods and avoid conditionals.

(the emphasis is mine)

From the quote above, it's clear that Optional was introduced in the JDK to provide a limited mechanism for return types. Any other cases like using optional as a field, storing it into a Collection, creating an optional to substitute a null-check or/and in order to chain methods on it are considered to be an abuse of Optional.

Also have a look at another answer by Stuart Marks: Should Optional.ofNullable() be used for null check?

That said, the cleaner way of retrieving employee by id, when id in the request object can be null would be the following:

public Employee getEmployeeById(Request request) {
    Long id = request.getId();
    
    if (id == null) return new Employee();

    return employeeRepository.findById(id)
            .orElse(new Employee());
}

If fill uncomfortable that new Employee() appears twice in the code, then you can reimplement it like that:

public Employee getEmployeeById(Request request) {
    Long id = request.getId();        
    Optional<Employee> result = Optional.empty();

    if (id != null) result = employeeRepository.findById(id);

    return id == null || result.isEmpty() ? new Employee() : result.get();
}

Answered By - Alexander Ivanchenko
Answer Checked By - Marie Seifert (JavaFixing Admin)