Why is Optional.get() without calling isPresent() bad, but not iterator.next()?

Question

When using the new Java8 streams api to find one specific element in a collection, I write code like this:

    String theFirstString = myCollection.stream()
            .findFirst()
            .get();

Here IntelliJ warns that get() is called without checking isPresent() first.

However, this code:

    String theFirstString = myCollection.iterator().next();

...yields no warning.

Is there some profound difference between the two techniques, that makes the streaming approach more "dangerous" in some way, that it is crucial to never call get() without calling isPresent() first? Googling around, I find articles talking about how programmers are careless with Optional<> and assume they can call get() whenever.

Thing is, I like to get an exception thrown at me as close as possible to the place to where my code is buggy, which in this case would be where I am calling get() when there is no element to be found. I don't want to have to write useless essays like:

    Optional<String> firstStream = myCollection.stream()
            .findFirst();
    if (!firstStream.isPresent()) {
        throw new IllegalStateException("There is no first string even though I totally expected there to be! <sarcasm>This exception is much more useful than NoSuchElementException</sarcasm>");
    }
    String theFirstString = firstStream.get();

...unless there is some danger in provoking exceptions from get() that I am unaware of?

---

After reading the response from Karl Bielefeldt and a comment from Hulk, I realized my exception code above was a bit clumsy, here's something better:

    String errorString = "There is no first string even though I totally expected there to be! <sarcasm>This exception is much more useful than NoSuchElementException</sarcasm>";
    String firstString = myCollection.stream()
            .findFirst()
            .orElseThrow(() -> new IllegalStateException(errorString));

This looks more useful and will probably come natural in many places. I still feel like I will want to be able to pick one element from a list without having to deal with all of this, but that could just be that I need to get used to these kinds of constructs.

Answer 1

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.

Answer 2

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.

Answer 3

I agree that there is no inherent difference in these, however, I'd like to point out a bigger flaw.

In both cases, you have a type, which provides a method that is not guaranteed to work and you are supposed to call another method before that. Whether your IDE/compiler/etc warns you about one case or the other just depends on whether it supports this case and/or you have it configured to do so.

That being said though, both pieces of code are code smells. These expressions hint at underlying design flaws and yield brittle code.

You are correct in wanting to fail fast of course, but the solution at least for me, cannot be to just shrug it off and throw your hands in the air (or an exception onto the stack).

Your collection may be known to be non-empty for now, but all you can really rely on is its type: Collection<T> - and that doesn't guarantee you non-emptiness. Even though you know it now to be non-empty, a changed requirement may lead to the code upfront being changed and all of a sudden your code is hit by an empty collection.

Now you are free to push back and tell others that you were supposed to get a non-empty list. You can be happy that you find that 'error' fast. Nevertheless, you have a broken piece of software and need to change your code.

Contrast this with a more pragmatic approach: Since you get a collection and it can possibly be empty, you force yourself to deal with that case by using Optional#map, #orElse or any other of these methods, except #get.

The compiler does as much work as possible for you such that you can rely as much as possible on the static-type contract of getting a Collection<T>. When your code never violates this contract (such as via either of these two smelly call chains), your code is much less brittle to changing environmental conditions.

In the above scenario, a change yielding an empty input collection would be of no consequence whatsoever for your code. It's just another valid input and thus still handled correctly.

The cost of this is that you have to invest time into better designs, as opposed to a) risking brittle code or b) unnecessarily complicating things by throwing exceptions around.

On a final note: since not all IDEs/compilers warn about iterator().next() or optional.get() calls without the prior checks, such code is generally flagged in reviews. For what it's worth, I would not let such a code allow to pass a review and demand a clean solution instead.