squid:S3655 is it correct?

java

(Raffig) #1

The rule says that this:

Optional<String> value = this.getOptionalValue();

// ...

String stringValue = value.get(); // Noncompliant

Is not compliant. My question is: why? I’ve read the discussion on previous forum, that this is due to multithreading. But Optional is immutable, so it is thread safe. Why rule expects to check if value is present before every call to get? Besides even if optional wouldn’t have been immutable it still makes no sense as doing such check is not atomic, thus still:

if (opt.isPresent) opt.get();

could throw an exception.

Isn’t this rule based on erroneous assumptions?

Best regards!
Rafal


(Tibor Blenessy) #2

Hello,

the value should be checked, because if Optional doesn’t contain the value you will have NoSuchElementException thrown when this code is run.

Multithreading and immutability have nothing to do with it. I am not sure to which discussion do you refer, if you believe it’s relevant, please send a link.


(Raffig) #3

OK, ma bad. I misunderstood the case that was used as example in the discussion I recalled :-). In fact the explanation of rule is also a little bit confusing as it says:

> To avoid the exception, calling the isPresent() method should always be done before any call to get().

and the example:

Optional<String> value = this.getOptionalValue();

// ...

if (value.isPresent()) {
  String stringValue = value.get();
}

Suggest that it is necesary to do isPresent every time right before get().

After your explanations I did some checks and it seems that rule works fine. The only case when it can be debatable is when I know that in my business case/API returned optional is always nonempty. However sonar of course can know nothing about it, so doing something like:

getOptional().orElseThrow(() -> new UnexpectedMissingValue());

Should be good enough.

Sorry for this misunderstanding.

Best regards!
Rafal


(Tibor Blenessy) #4