The explanation for the rule states:
When the call to a function doesn’t have any side effects, what is the point of making the call if the results are ignored?
However, orElseThrow does indeed have side effects. It throws an exception if the result is null.
Optional.ofNullable(null).orElseThrow();Hey @lbenedetto ,
Thanks for the reproducer. I’m however not sure that I agree with you.
For me, your code does not make sense in practice (I might need more context), because not using the value of the Optional seems like an antipattern, defeating the purpose of the Optional class. Is it a hack to avoid manually throw an exception?
Also, why even instantiate an Optional for the sole purpose of a simple (and less costly in memory) null verification? Are you using this syntax solely to avoid the code described in bar() below?
void foo(@Nullable String o) {
Optional.ofNullable(o).orElseThrow();
}
void bar(@Nullable String o) {
if (o == null) {
// allows a much more specific and contextual Exception message
// compared to the generic "No value present" from Optional
throw new NoSuchElementException("parameter 'o' is null and it should not have been the case!");
}
}
In my mind, I would expect at least to assign the value to something of that the optional will be accessed at some point later, the orElseThrow() anyway making sure that any missing values break the flow. Summarized, the following code of foo() feels absurd compared to bar():
void foo() {
Optional<String> o = getOptional();
o.orElseThrow(); // issue raised by S2201
// ...
// do some stuff
// ...
String s = o.get();
// use 's' later on
}
void bar() {
Optional<String> o = getOptional();
String s = o.orElseThrow(); // do not raise issue by S2201
// ...
// do some stuff
// ...
// use 's' later on ... at worth the variable will be garbage collected
}
I would appreciate your opinion or a detailed use case, in case I missed the point! 
Thanks,
Michael
I thought the point was to provide a minimal reproducing example, not a realistic use case.
Of course I’m not just instantiating an optional to immediately call orElseThrow. It’s more like a method that accepts an optional and does some mapping/filtering and finally terminates with an orElseThrow. And that’s the end of a void return method.
The point is, the reasoning for the rule is that you shouldn’t call methods without side effects and not use the return value. This method does have side effects, so it can make sense to call it without using the return value.
I came upon this discussion and I believe the reporter has a valid issue. Since you took exception to their minimal example, here’s a use case that I believe is valid but fails Sonar:
Optional.ofNullable(dbConnector.getMapper().findCustomerId(customerId)).orElseThrow(DataNotFoundException::new);
,The sole purpose of this line of code is to ascertain the existence of a duplicate ID in a database. We do not care about the returned record. We simply want to use the declarative API of java.util.Optional to catch this value and throw an exception when it does not exist.
Sonar incorrectly flags this code as non compliant and “useless” in java:S2201
orElseThrow has side effects, which alone contradicts the explanation for S2201.
If not using the value of an Optional “seems like an antipattern” , then in my opinion it should be a separate rule*. But having a side effect by itself makes including orElseThrow as part of S2201 contradictory.
*And of course my agenda of making it a separate rule is because that would allow me to disable that rule. In my opinion, the code is much cleaner without the if. In its current state I am feeling forced to suggest to fellow devs in my team to SuppressWarnings for S2201 when this warning appears. S2201 by itself is useful and does deserve to be classified as a bug. But “not using the value of an Optional” sounds more like something that’d belong in the code smell category.
Hi @vexorian,
Welcome to the community!
That’s a good point. We shouldn’t include orElseThrow in S2201 but create a separate rule for it. I’ve created a ticket, and we’ll use the rule telemetry data to decide how we want to solve the problem. For now, the best option is to disable the rule.
Best,
Erwan
Great news! The ticket has been implemented, and we no longer raise errors when orElseThrow is used. This update should be soon available on SonarQube for Cloud and released with SonarQube Server 10.9.
This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.