Calling Optional#isPresent() in single line is reported as not called

Question

I run SonarQube to check my code and I found a case which I don't understand the reported error.

My code is:

private static final int BASE_ID = 100_000_000;
private boolean isValidId(Id id) {
    return id.asInteger().isPresent() && id.asInteger().get() >= BASE_ID;
}

The method asInteger returns Optional<Integer>

The error that I am getting from sonarqube is Call "Optional#isPresent()" before accessing the value. in the return line.

I understand that the code is ok as the second part of the if won’t get executed if the first one is false. I know that this can be solved with a .filter(..).isPresent() but I like it more this way.

Any ideas why would this happen?

I'd say it's just because those code inspection tools don't always work perfectly and some errors are just false positives. IF you rewrite it as `if(...)` I'm sure the error won't be reported anymore — UninformedUser, Nov 06 '18 at 20:42
What if second call to `id.asInteger()` would return empty `Optional`? — tsolakp, Nov 06 '18 at 20:44

Andreas · Accepted Answer · 2018-11-06T21:06:33.110

20

Sonarqube cannot guarantee that the two calls to id.asInteger() returns the same object, e.g. because multi-threading might have changed the value of id between the two calls, so it is correctly stating that the presence hasn't been adequately tested.

Change code to assign to a local variable first, to ensure that isPresent() and get() are called on the same object:

private boolean isValidId(Id id) {
    Optional<Integer> idAsInteger = id.asInteger();
    return idAsInteger.isPresent() && idAsInteger.get() >= BASE_ID;
}

edited Nov 06 '18 at 21:06

answered Nov 06 '18 at 20:44

Andreas

154,647
11
152
247

13

Multi-threading isn't even needed: two calls to the same method are not guaranteed to return the same thing, and I'm not sure Sonar is able to guarantee that asInteger() is idempotent. – JB Nizet Nov 06 '18 at 20:46
1

@Andreas excellent point, 1+, but I would completely write that as a single statement though – Eugene Nov 06 '18 at 20:47
3

@JBNizet Sonar can never guaranty any properties of a method in a different class, as it can’t even guaranty that the implementation will be the same at runtime. The only exceptions would be well known (JDK) methods with a precisely defined contract or formal contracts expressed as annotations. – Holger Nov 07 '18 at 07:15

score 5 · Answer 2 · answered Nov 06 '18 at 20:42

5

You can write that as a single statement btw:

return id.asInteger()
         .map(x -> x >= BASE_ID)
         .orElse(false)

but sonar complaining is because well it's a false positive in this case.

answered Nov 06 '18 at 20:42

Eugene

117,005
15
201
306

Yes I know I can solve it with different methods, like the one I wrote in the question itself: `.filter(..).isPresent()` but I like it more the other way and want to understand why this may be happening. I mean, the isPresent() is being called... – Pablo Matias Gomez Nov 06 '18 at 20:44
2

But SonarQube can't know if each call to asInteger() returns a different value. So the second time, when you call get(), it might be an empty Optional – EduSanCon Nov 06 '18 at 20:47
1

It is not a false positive - Sonar is completely correct to highlight this as a potential issue – Hulk Nov 07 '18 at 07:57

score 0 · Answer 3 · answered Nov 06 '18 at 20:54

0

When working with Optionals you should avoid .isPresent and .get as much as possible. The use of these methods in not safer than using nulls and is against the functional spirit. Optionals are made for functional programming and for a typesafe alternative of null checks.

SonarQube has only limiteds capabilities with its analysis. It usually cannot exclude all kinds of false positives. This case is not really a problem because it is not recommended to use Optionals this way.

answered Nov 06 '18 at 20:54

Donat

4,157
3
11
26

Not necessary. For example how would you not use `isPresent` when you want to take some action when value is not present. – tsolakp Nov 06 '18 at 20:59
@tsolakp Since Java 9 `yourOptional.ifPresentOrElse(dummy, this::someAction);`. Your point is valid, though, it is one of the *rare* (and possibly even sought-after?) cases where one may defend the use of `isPresent()`. – Ole V.V. Nov 11 '18 at 12:55
1

Yes, my intention was not to say "you must not use isPresent", but "in most cases it would be better to avoid it, if possible". – Donat Nov 11 '18 at 13:26

score 0 · Answer 4 · answered Mar 21 '19 at 20:21

0

To avoid this problem I use iterator().next() it has the same functionality that .get but it cames without the isPresent() issue!

answered Mar 21 '19 at 20:21

Leandro Maro

325
1
12

Calling Optional#isPresent() in single line is reported as not called

4 Answers4