Which rule?
java:S2789 “null” should not be used with “Optional”
Why do you believe it’s a false-positive?
I have a simple class contains the m map data member:
class Foo {
// map string to optional, I need to know three cases:
// key is not in map
// key value is not null
// key value is null
Map<String, Optional<Object>> m = new HashMap<>();
public Object value(String key) {
synchonized(m) {
Optional<Object> o = m.get(key);
if( o == null ) { // Ensure this "Optional" could never be null and remove this null-check.
o = Optional.ofNullable(calcWithRecursiveModificationOfMap(key));
m.put(key, o);
}
return o.orElse(null);
}
}
}
As for me here is false positive case, because Map.get can return null
Are you using
SonarQube - which version?
v9.9.5 (build 90363) Community Edition
How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
The code snipped you shared is an interesting conceptual experiment that shows how a misuse of APIs can lead to inconsistency.
The intent of this code is not clear:
What is the reason for having an Optional as a value type in a Map? As mentioned in this blog and referenced from Effective Java:
it is almost never appropriate to use an optional as a key, value, or element in a collection or array
The function seems to be creating and adding a new element when it is not present in the map: why aren’t you using the Map APIs? A more reasonable implementation would be:
if(!m.containsKey(key) {
o = Optional.ofNullable(calcWithRecursiveModificationOfMap(key));
m.put(key, o);
}
or event better:
m.computeIfAbsent(key, k -> Optional.ofNullable(calcWithRecursiveModificationOfMap(k)));
I would suggest you to always opt for the simplest solution and to use properly the available APIs. The Map API provides methods that abstract the existence of nullable values, i.e. containsKey(Object), putIfAbsent(Object, Object) and computeIfAbsent(Object, Supplier).
Regarding the rule S2789 it enforces the good practice to not combine null checks with Optionals.
in my case value of key can be null, Optional allows me to understand it. if will use Map<String, Object> for any null value of key it will be recalculated. I can use containsKey, but in this case I have to search key twice: containsKey + get.
The code snippet you shared doesn’t call the function that modifies the map recursively, so the use of computeIfAbsent is valid and would not result in any ConcurrentModificationException.
I don’t see a problem in combining containsKey and get. You have a very specific use case that may be tackled in a different way than using Optional, like keeping track of nullable keys in a set and checking it or customizing your object with a method that specifies if it is present or not.
I’m afraid that this cannot be considered a false positive for rule S2789.