aherbert commented on code in PR #1063:
URL: https://github.com/apache/commons-lang/pull/1063#discussion_r1224468663
##########
src/main/java/org/apache/commons/lang3/ObjectUtils.java:
##########
@@ -595,7 +595,11 @@ public static short CONST_SHORT(final int v) {
* TODO Rename to getIfNull in 4.0
*/
public static <T> T defaultIfNull(final T object, final T defaultValue) {
- return object != null ? object : defaultValue;
+ try {
Review Comment:
The suggested method would work. However this is outside of the scope of the
library. There are many uses of a `Supplier` in the library and we do not want
to set a precedent for catching NPE and handling this by returning `null`.
A possible change that may be acceptable is to update
`o.a.c.l.function.Suppliers.get` to catch NPE:
```Java
public static <T> T get(final Supplier<T> supplier) {
try {
return supplier == null ? null : supplier.get();
} catch (NullPointerException ignored) {
return null;
}
}
```
Then you can call e.g.:
```Java
List<String> list = Collections.emptyList();
Object object = ObjectUtils.defaultIfNull(Suppliers.get(() -> list.get(0)),
"helloDefault");
```
The `Suppliers` class then effectively hides your potential NPE.
I would not recommend this change to `Suppliers`. A NPE is typically
generated by a programming error and this type of usage would cover it up. The
use case may be that a supplier is provided by an unknown party and you wish to
protect against generating NPE. But a third party could throw any type of RTE
or error. Only catching NPE is a specific case where you are using your own
supplier and are aware it may dereference null. This seems like it should be
handled better in the original implementing code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
