CODEWITHSUNDEEP
javaoption-type
Aarkan
Aarkan

Reputation: 4109

Using Optional in Java 8

I want to construct an Employee object using an employeeId and if the employeeId is not found, just print a message. Based on my understanding, I came up with following:

private Optional<Employee> createEmployee(SearchParams params) {

Optional<String> employeeId = searchEmpId(params);

Optional<Employee> employeeOptional = employeeId.map(epmId -> new Employee());
employeeOptional.ifPresent(employee -> {
  employee.setName(getEmpName(employeeId));
  employee.setSalary(getEmpSalary(employeeId));
});

employeeOptional.orElseGet(() -> {
  LOG.warn("No emp records found for params {}.", params);
  return null;
});

return employeeOptional;
}

Is there a way to simplify/optimize the above code? I believe there should be a simple way to do that. Any help is much appreciated.

Upvotes: 0

Views: 667

Answers (4)

nndru
nndru

Reputation: 2107

You can go further and implement Martin Fowler's Special case pattern for Employee entity. But, main drawback here is that all Employee related stuff should be done within somof its methods, because only appropriate Employee instance should decide what to do in particaular case and this behaviour should not be externalized.

Upvotes: 0

Gerald M&#252;cke
Gerald M&#252;cke

Reputation: 11122

Basically this way

Optional<Employee> createEmployee(SearchParams params) {
  return searchEmpId(params).map(id -> {
   Employee e = new Employee();
   e.setName(getEmpName(id));
   e.setSalary(getEmpSalary(id));
   return e;
 })

The case with an empty employee is an exception to your happy flow and should be implemented and dealt with that way. The point of doing that is to avoid explicit calls to

if(!empOptional.isPresent() {...}

to either write a log statement or return an empty optional because you have to deal with the empty optional later in the control flow in any case.

So better define an exceptional flow, for example

try {
  Employee e = createEmployee(params).orElseThrow(() -> new EmployeeNotFoundException());
  e.practice("java");
} catch(EmployeeNotFoundExceptione e){
  LOG.info("not found, 404, whatever", e);
}

Of course you could handle that exception on an upper layer as well, just as it's appropriate to your case.

Upvotes: 1

Joeri Hendrickx
Joeri Hendrickx

Reputation: 17435

The way to make it cleaner is to not use the optional internally, since you don't use it in any useful way.

If you want to return an optional to the invoker, so that they can react accordingly, then fine. But the way you're using it internally doesn't make the code clearer in any form. So here's a suggestion:

private Optional<Employee> createEmployee(SearchParams params) {

  String employeeId = searchEmpId(params);

  if (employeeId == null) {
    LOG.warn("No emp records found for params {}.", params);
    return Optional.empty();
  }

  Employee employee = new Employee());
  employee.setName(getEmpName(employeeId));
  employee.setSalary(getEmpSalary(employeeId));
  return Optional.of(employee);
}

As an aside nitpick: if you want to return an Optional to the invoking class, that basically tells the invoking class 'this could be empty, and it's up to you to handle that'. In that case I would consider it bad form to log a warning youself. It's implied that the calling code will handle this issue, so do the logging. I would do internal logging here on debug level.

Upvotes: 0

fabriziocucci
fabriziocucci

Reputation: 782

Assuming you have a constructor that accept an id in the Employee class:

Optional<Employee> employee = employeeId.map(Employee::new)
    .map(employee -> employee.setName(...))
    .map(employee -> employee.setSalary(...));

if (!employee.isPresent()) {
    // log
}

return employee;

Upvotes: 0

Related Questions