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?

CodePudding user response：

As mentioned in the comments, the Optional doesn't do anything in this case. To solve it you need to handle the null value by yourself. You could do this by making the request.getId() return an Optional by itself (since it can be null).

Now you can chain these Optional's using the flatMap() operator.

public class MyRequestClass {
    private Long id;

    // Change your getter to this, or make a new one, eg. 'getOptionalId()'
    public Optional<Long> getId() {
        return Optional.ofNullable(id);
    }
}

Now you can refactor your code to:

Employee employee = request
    .getId()
    .flatMap(employeeRepository::findById) // Using flatMap() in stead of map()
    .orElseGet(Employee::new); // I changed this part so that new Employee() is 
                               // called lazily when no Employee was found in 
                               // the database (or when the request has no ID)

CodePudding user response：

How about null-handling of the request.getId() and avoiding falling into the exception?

Use one of the following, assuming employeeRepository#findById returns Optional<Employee>:

Employee employee = Optional.ofNullable(request.getId())
                            .flatMap(id -> employeeRepository.findById(id))
                            .orElse(new Employee());

Employee employee = Optional.ofNullable(request.getId())
                            .flatMap(EmployeeRepository::findById)
                            .ofElse(new Employee());

CodePudding user response：

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();
}