Is accepting Optional as argument a code smell?
I was reviewing some piece of code and came across a function that was taking Optional as argument. In the same class, another function was taking a couple of Optional as arguments.
When I thought about it, I felt that taking an Optional should be avoided. You can return an Optional from a function, but avoid taking Optional arguments. Before calling a function, you should check for the presence or absence of the values that you are passing. Hence a function taking one or more Optional is a code smell.
One argument that can possibly presented in favor of taking Optional as argument is every caller checking for the presence of the arguments. Consider the code example below:
If you pay attention, the error handling from the caller's point of view is ugly if the caller wants to report correct error. Much worse, the error check is done after using the values.
A side effect of product() taking Optional is that it must return an Optional. Otherwise it has to throw an exception. They both can be avoided if it doesn't take the Optional argument.
A simplified version will be:
When I thought about it, I felt that taking an Optional
One argument that can possibly presented in favor of taking Optional
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
/** | |
* Computes the product of the given Integer arguments. | |
* If any of the arguments are missing, it will return empty as result. | |
*/ | |
Optional<Integer> product(Optional<Integer> value1, Optional<Integer> value2) { | |
if(value1.isPresent() && value2.isPresent()) { | |
return Optional.of(value1.get().intValue()*value2.get().intValue()); | |
} else { | |
return Optional.empty(); | |
} | |
} | |
// Calling place. | |
Optional<Integer> value1 = computeValue1(); | |
Optional<Integer> value2 = computeValue2(); | |
Optional<Integer> result = product(value1, value2); | |
if(!result.isPresent()) { | |
// Identify which argument is not valid. | |
if(!value1.isPresent()) { | |
System.out.println("value1 is missing."); | |
} else { | |
System.out.println("value2 is missing."); | |
} | |
return Optional.empty(); | |
} |
If you pay attention, the error handling from the caller's point of view is ugly if the caller wants to report correct error. Much worse, the error check is done after using the values.
A side effect of product() taking Optional
A simplified version will be:
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
// No Optional arguments, hence no Optional return value. | |
Integer product(Integer value1, Integer value2) { | |
return value1.intValue()*value2.intValue(); | |
} | |
// Calling place is same as earlier, but presence check is done | |
// as soon as the values are computed before calling product() | |
// function. | |
Optional<Integer> value1 = computeValue1(); | |
if(!value1.isPresent()) { | |
System.out.println("value1 is missing."); | |
return Optional.empty(); | |
} | |
// If value1 is missing we may not even have to compute value2. | |
Optional<Integer> value2 = computeValue2(); | |
if(!value2.isPresent()) { | |
System.out.println("value2 is missing."); | |
return Optional.empty(); | |
} | |
Integer result = product(value1.get(), value2.get()); |
Comments