23

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.

  • 7
    The exception hrowing part form your last example could be written a lot more concise if you use orElseThrow - and it would be clear to all readers that you intend the exception to be thrown. – Hulk May 19 '16 at 15:16
12

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.

  • Could be I need to learn more about this Optional business - I've mostly seen java8 stuff as a nice way to do the mapping of lists of objects that we do a lot in our webapp. So you're supposed to send Optional<>s everywhere? That will take some getting used to, but that's another SE post! :) – TV's Frank May 19 '16 at 13:53
  • @TV's Frank It's less that they're supposed to be everywhere, and more that they allow you to write functions that transform a value of the contained type, and you chain them with map or flatMap. Optional's are mostly a great way to avoid having to bracket everything with null checks. – Morgen May 19 '16 at 16:14
15

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.

6

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.

  • I guess I can agree a bit about code smell - I made the example above to make a short post. Another more valid case might be to pluck a mandatory element from a list, which is not a strange thing to appear in an application IMO. – TV's Frank May 19 '16 at 13:04
  • Spot on. "Pick the element with property p from the list" -> list.stream().filter(p).findFirst().. and yet again we are forced to ask the question "what if it isn't in there?".. daily bread and butter. If it's mandatoray and "just there" - why did you hide it in a bunch of other stuff in the first place? – Frank May 19 '16 at 13:26
  • 4
    Because maybe it's a list that is loaded from a database. Maybe the list is a statistical collection that was gathered in some other function: we know we have objects A, B and C, I want to find the amount of whatever for B. Many valid (and in my application, common) cases. – TV's Frank May 19 '16 at 13:42
  • I don't know about your database, but mine doesn't guarantee at least one result per query. Same for statistical collections - who is to say they will always be non-empty? I agree, that it is straightforward to reason that there should be this or that element, yet I prefer proper static typing over documentation or worse some insight from a discussion. – Frank Jun 6 '16 at 5:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.