I am refactoring the code to Java 8 and I want to replace null checks with Optional.
public Employee findEmployeeById(String id) {
List<Employee> empList = .. //some db query
return (empList.isEmpty() ? null : empList.get(0));
}
Optional.ofNullable(empList.get(0)) won't work as when it will throw IndexOutofBoundException
Or should I ideally replace null with Optional.empty()?
As @Jesper already mentioned in the comments, you have to check whether the list is empty and then return an empty Optional.
public Optional<Employee> findEmployeeById(String id) {
List<Employee> empList = .. //some db query
return empList.isEmpty() ? Optional.empty() : Optional.of(empList.get(0));
}
An Optional is a wrapper around a potentially null value that allows you to avoid explicitly checking for null when you use it.
Have a look at the Optional documentation to see what functionality it provides.
For example you can get an employee's name or "unknown" if it's absent, without checking for null:
Optional<Employee> emp = findEmployeeById(id);
String name = emp.map(Employee::getName).orElse("unknown");
You may read this post about Uses for Optional to see if it makes sense for you to use Optional.
Optional.ofNullable().To me the natural solution would be to keep your ? : construct as in Floern’s answer. If, however, you want to get rid of that, there is also an elegant solution without it:
public Optional<Employee> findEmployeeById(String id) {
List<Employee> empList = .. //some db query
return empList.stream().findFirst();
}
This gives you what you want because findFirst() returns an Optional. If you don’t care which element you get — or you know there’s never more than one — you may alternatively use findAny(), it too returns an Optional.
empList local, so this can be reduced to return someDbQuery().stream().findFirst().Why don't you simply replace your method with:
public Optional<Employee> findEmployeeById(String id) {
List<Employee> empList = .. //some db query
return (empList.isEmpty() ? Optional.empty() :
Optional.ofNullable(empList.get(0)));
}
I suggest you wrap the empList.get(0) in a Optional.ofNullable in case it might still be null.
As far as why that is better: think about the caller of the method. Whomever is now calling your method has to think what to actually do when the result is empty.
Besides you are now forced into writing code like:
Optional<Employee> emp = findEmployeeById("12");
if (emp.isPresent()) {
} else {
....
}
You can also chain this to become more fluent like:
emp.orElseThrow(RuntimeException::new)
Or other Optional methods.
That is simply not the case when you return an Employee. You are not even thinking (usually) to check if the reference is null.
That makes your code less error-prone and easier to understand.
Another possibility would be to do it as follows:
return Optional.of(empList).filter(list -> !list.isEmpty()).map(list -> list.get(0));
This will automatically return an empty Optional in case the list is empty or empList.get(0) returns null.
If empList can be null, consider using Optional.ofNullable(empList) instead of Optional(empList).
return empList.isEmpty() ? Optional.empty() : Optional.of(empList.get(0));