mebigfatguy / fb-contrib

a FindBugs/SpotBugs plugin for doing static code analysis for java code bases
http://fb-contrib.sf.net
GNU Lesser General Public License v2.1
155 stars 45 forks source link

Optional orElse vs orElseGet #179

Closed mattnelson closed 7 years ago

mattnelson commented 7 years ago

The JDK8 optional implementation contains two orElse* methods that when used incorrectly have a high likelihood of unintended side effects. I have started to always use orElseGet even in simple scenarios. I think it would be helpful to have a detector that always preferred the usage of orElseGet or was able to detect when orElse was used with a method call.

// side effect flow
opt.orElse(expensiveOperation()); // bug
opt.orElseGet(() -> expensiveOperation());

opt.orElse(String.format("%s_%s", valOne, valTwo)); // bug
opt.orElseGet(() -> String.format("%s_%s", valOne, valTwo));

// simple flow
opt.orElse("<unknown>");
opt.orElseGet(() -> "<unknown>");

https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#orElse-T- https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#orElseGet-java.util.function.Supplier-

mebigfatguy commented 7 years ago

thanks for the idea. seems doable

ThrawnCA commented 7 years ago

Question: does this implementation handle autoboxing? I can easily imagine someone using javaLangIntegerOptional.orElse(0). The valueOf method can probably be considered to have negligible costs.

mebigfatguy commented 7 years ago

yeah, i also will rule out a + b (strings)

mattnelson commented 7 years ago

There are optionals designed for primitives. These should also be checked for proper orElse* usage.

Unrelated to the original intention of this issue, but another rule could be introduced to recommend OptionalDouble over Optional<Double>

https://docs.oracle.com/javase/8/docs/api/java/util/OptionalDouble.html https://docs.oracle.com/javase/8/docs/api/java/util/OptionalInt.html https://docs.oracle.com/javase/8/docs/api/java/util/OptionalLong.html

mebigfatguy commented 7 years ago

i guess i'll close this. if anyone has more ideas, or finds issues, feel free to re-open. Thanks for the idea.