Replace get() with orElse(null).
First of all, consider that the check is complex and probably relatively time consuming to do. It requires some static analysis, and there might be quite a bit of code between the two calls.
Second, the idiomatic usage of the two constructs are very different. I program full time in Scala, which uses Options much more frequently than Java does. I can't recall a single instance where I needed to use a .get() on an Option. You should almost always be returning the Optional from your function, or be using orElse() instead. These are much superior ways to handle missing values than throwing exceptions.
Because .get() should rarely be called in normal usage, it makes sense for an IDE to start a complex check when it sees a .get() to see if it was used properly. In contrast, iterator.next() is the proper and ubiquitous usage of an iterator.
In other words, it's not a matter of one being bad and the other not, it's a matter of one being a common case that's fundamental to its operation and is highly likely to throw an exception during developer testing, and the other being a rarely used case that may not be exercised enough to throw an exception during developer testing. Those are the sorts of cases you want IDEs to warn you about.
The Optional class is intended to be used when it is not known whether or not the item it contains is present. The warning exists to notify programmers that there is an additional possible code path that they may be able to handle gracefully. The idea is to avoid exceptions being thrown at all. The use case you present is not what it is designed for, and therefore the warning is irrelevant to you - if you actually want an exception to be thrown, go ahead and disable it (although only for this line, not globally - you should be making the decision as to whether or not to handle the not present case on an instance by instance basis, and the warning will remind you to do this.
Arguably, a similar warning should be generated for iterators. However, it has simply never been done, and adding one now would probably cause too many warnings in existing projects to be a good idea.
Also note that throwing your own exception can be more useful than just a default exception, as including a description of what element was expected to exist but didn't can be very helpful in debugging at a later point.
Disclosure: I'm an IntelliJ IDEA developer, responsible for this feature.
Technically, it's possible to cause get() failing using your code like this:
public static void main(String[] args) {
new Example.OneMoreClass().method1(new Example.Foo() {
int x;
@Override
public Optional<String> bar() {
return x++ % 2 == 0 ? Optional.of("foo") : Optional.empty();
}
});
}
As your Foo class and bar() method are not final, IDEA cannot be sure that they are stable. However, IDEA still warns even if you declare Foo as final. We actually trust that bar() result could be stable, to avoid noise warnings. The main problem is that an isPresent() check is moved into the separate method and our inter-procedural analysis is not that smart. Current analyzer implementation does only a few things when an unknown user method is called:
We infer result nullability, result mutability (in very limited sense), method purity and contracts like
@Contract("null -> false")looking into method implementation. This analysis is pretty limited and applied to non-overridable methods only, so it doesn't apply here. Even if we declaremethod2as final, this analysis yields nothing.We infer not-null parameters, looking into method implementation. This works in your case: if you call
method2(null), you will get a warning. Of course, it's not helpful to solve your problem.In some cases, we inline very small and simple stable methods. This works only if method to be inlined is stable, has no arguments, called on the same class, has single return statement and doesn't call any other methods (probably there are more restrictions). It helps in the cases like this:
public static final class OneMoreClass {
@Nullable String foo;
void test() {
if (isValid()) {
System.out.println(foo.trim()); // no possible NPE warning
}
}
boolean isValid() {
return foo != null;
}
}
None of these approaches covers your case. We occasionally improve the analysis making it smarter, but we are pretty limited in CPU resources. Don't forget that we analyse the code online, and the code is constantly edited invalidating the previous analysis results. Deep interprocedural analysis requires much more CPU time and we don't like making the IDEA significantly slower.
I must say that your code might confuse readers as well. I think that your sample is the simplified version of bigger code, and the reader might be not sure whether the optional is actually always present at given point of code. There's a good old mechanism to assist code readers as well as protect yourself against accidental mistakes (what if somebody modifies method2 later removing the isPresent() check without proper adaptation of call sites?). That mechanism is called assertions. So the answer: add an assert, and the code would be more clear both for IDE, and for readers:
void method1(final Foo foo) {
method2(foo);
assert foo.bar().isPresent();
System.out.println(foo.bar().get()); // the warning is gone
}
You're not going to be able to get rid of the warning in this instance. The code inspection that generates warning is not robust enough to completely analyze all code paths and understand the intent behind method2.
You could do something like this, though, to achieve the same result:
void method1(final Foo foo) {
String bar = getBar(foo);
System.out.println(bar);
}
String getBar(final Foo foo) {
return foo.bar().orElseThrow(() -> new IllegalArgumentException("No value"));
}
Though honestly, now that I've been reminded of the .orElseThrow method, it's probably easier to simplify this to just:
void method1(final Foo foo) {
String bar = foo.bar().orElseThrow(() -> new IllegalArgumentException("No value"));
System.out.println(bar);
}
You can also genericize some of the functionality. It doesn't buy you much in this case, but if there's a lot more code that you want to run that is triggered by the optional being empty, it might make sense. IntelliJ will give a warning here, too, which you can suppress:
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
<T> T unwrapOrThrow(Optional<T> optional) {
return optional.orElseThrow(() -> new IllegalArgumentException("No value"));
}