128

We have recently moved to Java 8. Now, I see applications flooded with Optional objects.

Before Java 8 (Style 1)

Employee employee = employeeServive.getEmployee();

if(employee!=null){
    System.out.println(employee.getId());
}

After Java 8 (Style 2)

Optional<Employee> employeeOptional = Optional.ofNullable(employeeService.getEmployee());
if(employeeOptional.isPresent()){
    Employee employee = employeeOptional.get();
    System.out.println(employee.getId());
}

I see no added value of Optional<Employee> employeeOptional = employeeService.getEmployee(); when the service itself returns optional:

Coming from a Java 6 background, I see Style 1 as more clear and with fewer lines of code. Is there any real advantage I am missing here?

Consolidated from understanding from all answers and further research at blog

user3198603
  • 1,896
  • 2
  • 16
  • 21
  • 22
    Oracle has an [extensive article about the use of Optional](http://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html). – Mike Partridge Jan 18 '18 at 15:33
  • 31
    Yuck! `employeeOptional.isPresent()` seems to be entirely missing the point of option types. As per @MikePartridge's comment, this isn't recommended or idiomatic. Explicitly checking whether an option type is *something* or *nothing* is a no-no. You're supposed to simply `map` or `flatMap` over them. – Andres F. Jan 18 '18 at 19:43
  • 9
    See [a *Stack Overflow* Answer on `Optional`](https://stackoverflow.com/a/26328555/642706) by [Brian Goetz](https://stackoverflow.com/users/3553087/brian-goetz), the *Java Language Architect* at Oracle. – Basil Bourque Jan 19 '18 at 02:17
  • 1
    What's the behavior supposed to be when it *is not* present? In practice, it's extremely rare that you simply want to continue on and do nothing when a value is missing (regardless of whether you use `null` or `Optional` to determine whether its missing or not). – jpmc26 Jan 19 '18 at 02:53
  • 6
    This is what happens when you turn off your brain in the name of "best practices". – user253751 Jan 19 '18 at 04:41
  • 9
    That is fairly poor usage of optional but even that has benefits. With nulls **everything** may be null, so you need to constantly be checking, optional calls out that this particular variable is likely to be missing, make sure you check for it – Richard Tingle Jan 19 '18 at 06:58
  • So many answers, but no one properly describes advantages with "else" cases, e.g. `Employee e = employeeOptional.orElseThrow(NotFoundException::new)`. – Cedric Reichenbach Jan 19 '18 at 12:48
  • 2
    @RichardTingle I think you wanted to say 'With nulls everything may be null, so you *constantly keep forgetting to check*' :) – crizzis Jan 20 '18 at 20:35
  • The difference is that you cannot use the object of type `Optional` in the same way you use `T` (while this is not the case for `null`). Hence you are *forced* to handle this fact in your code. Sure you can still use `Optional::get` to trigger an NPE but at least it's your code that explicitly does so, and if you avoid it, it is very easy to write non-NPE generating code which is checked for correctness by the compiler. With `null`s every operation could explode but you'll know only at runtime. – Bakuriu Jan 21 '18 at 07:27
  • You may also want to read [Should I use Java8/Guava Optional for every method that may return null?](https://stackoverflow.com/a/18699418/697630) – edalorzo Jan 21 '18 at 13:33
  • Study up on Functional Programming. – Joshua Jones Jan 21 '18 at 20:20
  • Funny thing, why use Optional to avoid NullPointerException, when it does throw the same exception? – Majid Ali Khan Sep 02 '19 at 13:43
  • Before we had optional, we used to get NullPointerExceptions. Now we get NoSuchElement exceptions. That is real progress. – Tuntable Jan 01 '20 at 08:14
  • @Tuntable You only get NoSuchElement if you call `get`, which shouldn't be a public member – Caleth Mar 06 '20 at 14:00
  • @Tuntable that's only because people are calling `get`, which is an antipattern (like Caleth says, it shouldn't be a public method). Sadly, I think it's too hard to use `Optional` right in Java. – Andres F. Mar 06 '20 at 20:01

11 Answers11

125

Style 2 isn't going Java 8 enough to see the full benefit. You don't want the if ... use at all. See Oracle's examples. Taking their advice, we get:

Style 3

// Changed EmployeeServive to return an optional, no more nulls!
Optional<Employee> employee = employeeServive.getEmployee();
employee.ifPresent(e -> System.out.println(e.getId()));

Or a more lengthy snippet

Optional<Employee> employee = employeeServive.getEmployee();
// Sometimes an Employee has forgotten to write an up-to-date timesheet
Optional<Timesheet> timesheet = employee.flatMap(Employee::askForCurrentTimesheet); 
// We don't want to do the heavyweight action of creating a new estimate if it will just be discarded
client.bill(timesheet.orElseGet(EstimatedTimesheet::new));
Caleth
  • 10,519
  • 2
  • 23
  • 35
  • I think you'd want to use map(), not flatMap() - and use a method reference for bonus points – Michael Borgwardt Jan 18 '18 at 15:47
  • @MichaelBorgwardt the intention is that `Employee::askForCurrentTimesheet` returns `Optional` – Caleth Jan 18 '18 at 16:02
  • 19
    indeed we ban most uses of isPresent (caught by code review) – jk. Jan 18 '18 at 16:05
  • 11
    @jk. indeed, if there were an overload `void ifPresent(Consumer, Runnable)` I would argue for a total ban of `isPresent` and `get` – Caleth Jan 18 '18 at 16:23
  • 2
    And my point by that is - Java isn't Smalltalk, we have `if` statements and it's okay to use them. In Smalltalk everything (even `if` and `while`!) is a method call. – user253751 Jan 19 '18 at 04:42
  • 11
    @Caleth: You may be interested in Java 9's [`ifPresentOrElse(Consumer super T>, Runnable)`](https://docs.oracle.com/javase/9/docs/api/java/util/Optional.html#ifPresentOrElse-java.util.function.Consumer-java.lang.Runnable-). – wchargin Jan 19 '18 at 04:47
  • 4
    @Caleth: That said, not sure that I agree with your "total ban of `isPresent` and `get`". When you want to do something that isn't lambda-y—mutate locals or propagate exceptions, for instance—existing methods even in Java 9 will not suffice (AFAICT). – wchargin Jan 19 '18 at 04:48
  • 10
    I find one drawback in this java 8 style, compared to the java 6 one : it fools code coverage tools. With if statement, it shows when you missed a branch, not here. – Thierry Jan 19 '18 at 09:10
  • 3
    @Thierry Shouldn’t code coverage tools be able to discern whether the lambda was invoked? – Jonas Schäfer Jan 19 '18 at 10:39
  • @JonasWielicki not sure if it is possible or not (i'm no specialist of coverage technics), but it is clearly not done with the one i use (jacoco-0.7.9) – Thierry Jan 19 '18 at 12:15
  • 5
    Or, indeed, taking the "style 3" example a step further: `employeeService.getEmployee().map(Employee::getId).ifPresent(System.out::println);` – Jules Jan 19 '18 at 19:47
  • 6
    @Jules And *that* is one of the primary reasons I dislike this style of code. That line may make you feel smart when you write it, but it drastically reduces code readability. In a review, I would deny that hard and insist the style be dropped as a bad habit. – Aaron Jan 19 '18 at 19:59
  • 3
    @Jules I think Caleth purposely wrote verbosely for the gentle StackOverflow reader who would otherwise be unaware of the type. In real code, your style is preferable – emory Jan 19 '18 at 21:38
  • Hmm, looks like a version of C#'s null propagation. – IS4 Jan 19 '18 at 23:58
  • 1
    Well I liked this answer a lot and this is truly amazing in a sense where it tell how approach 2 can be further optimized. But my question was if I wrap the optional around single object , does it add any value as compared to traditional null check approach. Approach 3 tells which usecase Optional around single object actually makes sense. But again it truly helped me to know other stuff in brief and concise way. – user3198603 Jan 20 '18 at 08:44
  • 17
    @Aaron "drastically reduces code readability". What part exactly reduces readability? Sounds more like a "I've always done it differently and I don't want to change my habits" than objective reasoning. – Voo Jan 20 '18 at 12:35
  • @IllidanS4 In some ways yes, but `Optional` types are superior to simple null propagation in languages without non-nullable reference types because they allow you to specify in the type system that something might be null and force the caller to explicitly deal with it. – Voo Jan 20 '18 at 12:39
  • As a C# developer, this seems to make more sense than the OP. However, it seems to be overkill to call a lambda. C# seems simpler to have the ?. for null checks. – Doug Jan 20 '18 at 18:40
  • @thierry the ECLemma plugin in Eclipse (which I think is based on jacoco) has instruction counting as well, which will mark a line like `.map(x -> frobber.frob(x))` as only partially covered if the object wasn't present. However, `.map(frobber::frob)` is always completely covered. So it's a mixed bag. In some sense, it doesn't matter: the method should have been tested elsewhere, and tests shook include enough to cover all the cases anyway, so the lambdas should have a case where it's executed. But yeah, it's easy to write tests that "cover everything" without really covering everything. – Joshua Taylor Jan 21 '18 at 20:46
  • 1
    @Voo: To me, that part is the `e`. Introducing a new variable for an existing variable (`employee`) makes the code harder to reason about. *Especially* if you do this twice and name the variables differently. – user541686 Jan 22 '18 at 02:07
  • @Mehrdad Try using Scala. Most of the time, you'll be able to use underscores in short lambdas so you don't have to give your variables names like `e`. (BTW, the idea of avoiding variable names where they clutter the logic is called *pointfree* style.) – Brian McCutchon Jan 22 '18 at 06:01
  • @BrianMcCutchon: Underscores wouldn't nest readably though? And doing something like `_1`, `_2`, etc. gets even more confusing to reason about. – user541686 Jan 22 '18 at 07:58
  • 1
    @Voo Objective reason #1 (of many): Needlessly verbose with irrelevant/redundant stuff. Ex: we have employee but use `Employee` anyway instead of `employee.askForCurrentTimesheet()` added fluff makes `employee.flatMap(Employee::askForCurrentTimesheet)`. If you have to ask how "I do happen to want to possess one of the cookies. Were you to get one of the cookies for myself, I would be liking it." is not as easy to read - even if only from verbosity - than "I want a cookie. I would like it if you got one for me." then it'll be hard to discuss. – Aaron Jan 22 '18 at 15:23
  • 2
    @Aaron `employee.flatMap(Employee::askForCurrentTimesheet)` -> You might not have an employee, and even if you do, they might not have a timesheet for you. You end up with the possibility of a timesheet, but don't care *where* you failed to get it if there isn't one. There is exactly as much complexity as the domain demands. – Caleth Jan 22 '18 at 15:25
  • @Aaron and if `Employee::askForCurrentTimesheet` really grates you, you can switch it to `e -> e.askForCurrentTimesheet()` – Caleth Jan 22 '18 at 15:50
  • @Caleth I understand what is going on in the code. I am commenting only on the readability. There is not "exactly as much complexity as the domain demands". `if(employee != null && employee.getTimesheet() != null) client.bill(employee.getTimesheet()); else client.bill(new EstimatedTimesheet());` is easier to read. It even reads similar to how we would say it in English: "if the employee's not null and their timesheet's not null, client.bill their timesheet. Otherwise, bill an estimated timesheet." – Aaron Jan 22 '18 at 16:03
  • 1
    @Caleth The functional version is, for English speakers, like reading a language where the order of subjects, verbs, etc. are shifted around, and it is redundant in odd spots where you would not expect such. The fact that it is understandable does not mean that it is just as readable. Even if you train your brain to expect a different format that flows less naturally to be understood easier than one that flows more naturally, the language use in and of itself is still less readable. – Aaron Jan 22 '18 at 16:07
  • @Caleth That is, you could train your mind to read the programming language called Whitespace easier than it can read Java, or even Brainfuck, which is as dumb and messed up as its name suggests, but being personally capable of reading those easier than Java does not mean that they are in any way more readable than Java in and of themselves. – Aaron Jan 22 '18 at 16:08
  • 2
    You've got 3 seperate scopes, we have 1. You've repeated `client.bill` (or introduced a variable and assigned in a branch). Sounds more like a "I've always done it differently and I don't want to change my habits" than objective reasoning. People say the same kinds of things about `object.method(arg1, arg2, arg3)` and then write no-op "fluent interface" methods – Caleth Jan 22 '18 at 16:09
  • @Aaron You're aware of the irony of complaining about "needlessly verbose" and then showing an example that's noticeably shorter than the traditional procedural way to write it? You also introduce unnecessary extra scopes and create deeper nesting depths. If that's the best objective reason you can find, I'm not impressed. – Voo Jan 22 '18 at 22:33
  • @Mehrdad I'm talking about `option.map(_ + 1)` instead of `option.map(x => x + 1)`. It's a lambda shorthand language feature. – Brian McCutchon Jan 24 '18 at 03:42
50

If you're using Optional as a "compatibility" layer between an older API that may still return null, it may be helpful to create the (non-empty) Optional at the latest stage that you're sure that you have something. E.g., where you wrote:

Optional<Employee> employeeOptional = Optional.ofNullable(employeeService.getEmployee());
if(employeeOptional.isPresent()){
    Employee employeeOptional= employeeOptional.get();
    System.out.println(employee.getId());
}

I'd opt toward:

Optional.of(employeeService)                 // definitely have the service
        .map(EmployeeService::getEmployee)   // getEmployee() might return null
        .map(Employee::getId)                // get ID from employee if there is one
        .ifPresent(System.out::println);     // and if there is an ID, print it

The point is that you know that there's a non-null employee service, so you can wrap that up in an Optional with Optional.of(). Then, when you call getEmployee() on that, you may or may not get an employee. That employee may (or, possibly, may not) have an ID. Then, if you ended up with an ID, you want to print it.

There's no need to explicitly check for any null, presence, etc., in this code.

Joshua Taylor
  • 1,610
  • 11
  • 11
  • That's a very good point that I've never even thought about! <3 – Haakon Løtveit Jan 19 '18 at 13:50
  • There is an explicit check and it's `ifPresent` in the last line – edc65 Jan 20 '18 at 16:59
  • @edc65 That's a very implicit explicit check. And by that argument there are explicit checks in both map calls as well. Both of which you overlooked because they are very much not explicit - which is the whole idea behind Optionals. – Voo Jan 20 '18 at 17:26
  • @voo in fact I overllooked the implicit, but the explicit is very ... explicit indeed – edc65 Jan 20 '18 at 17:32
  • 7
    What is the advantage of using `(...).ifPresent(aMethod)` over `if((...).isPresent()) { aMethod(...); }`? It seems to simply be yet another syntax to do almost the same operation. – Ruslan Jan 20 '18 at 17:39
  • 2
    @Ruslan Why use one special syntax for one specific call when you already have a general solution that works equally well for all cases? We're just applying the same idea from map and co here as well. – Voo Jan 20 '18 at 17:44
  • @Ruslan I was just reproducing the behavior in OP's code. In practice, I'd be more likely to use the optional to get a default value if one isn't available, or to throw a specific exception. E.g., `EmployeeId id = Optional.of(...)...map(Employee::getId).orElseThrow(() -> new NoSuchEmployeeException("No employee named for name: "+name);` or `EmployeeId id = Optional.of(...)...map(Employee::getId).orElse(EmployeeId.NO_SUCH_EMPLOYEE);` or to compute a value instead: `.orElseGet(EmployeeId::createFreshUnusedId)`. For API compatibility, this is really handy when... – Joshua Taylor Jan 20 '18 at 17:53
  • 1
    @Ruslan ... an API returns nulls instead of empty collections or arrays. E.g., you can do something like (assuming a Parent class for which getChildren() returns a set of children, or null if there are no children): `Set children = Optional.of(parent).map(Parent::getChildren).orElseGet(Collections::emptySet);` Now I can process `children` uniformly, e.g., `children.forEach(relative::sendGift);`. Those could even be combined:`Optional.of(parent).map(Parent::getChildren).orElseGet(Collections::emptySet).forEach(relative::sendGift);`. All the boilerplate of null checking, etc., ... – Joshua Taylor Jan 20 '18 at 17:55
  • 1
    @Ruslan ... is delegated to tried and true code in the standard library. That reduces the amount of code that I, as a programmer, have to write, test, debug, etc. – Joshua Taylor Jan 20 '18 at 17:56
  • 1
    Glad I'm using Swift. if let employee = employeeService.employee { print (employee.Id); } – gnasher729 Jan 20 '18 at 20:01
  • @gnasher729 sure, as long as you remember to do that "if let", after remembering to check the docs to see that the method might return null (or similar). The Optional forces us to remember and handle it. – Joshua Taylor Jan 20 '18 at 20:33
  • @JoshuaTaylor Swift's `nil` (equivalent to `null`) is actually syntactic sugar for `Optional.none` (`T` is inferred). Only variables of `Optional` can be `nil`, so you can't ever "forget" to use `if let` (or some other unwrapping mechanism) – Alexander Jan 21 '18 at 03:47
  • @Alexander that's pretty nice, then. So `if let x = getEmployee() ...` really is a lot like `getEmployee().ifPresent(x -> { ... });`. It checks whether the value is there or not, and binds to a local variable in a new scope of it's there. Good stuff – Joshua Taylor Jan 21 '18 at 04:09
  • @JoshuaTaylor Yeah, it's really nice. It can confuse some people though, because you could do `if let x = x { ... }`, which produces a `T` from a `T?` (sugar for `Optional`), which shadows the outside `x`. It's convenient though, because it lets you avoid what would otherwise be arbitrary and necessarily-different names like `if let _x = x {...}` – Alexander Jan 21 '18 at 05:35
24

There is next to no added value in having an Optional of a single value. As you've seen, that just replaces a check for null with a check for presence.

There is huge added value in having a Collection of such things. All the streaming methods introduced to replace old-style low-level loops understand Optional things and do the right thing about them (not processing them or ultimately returning another unset Optional). If you deal with n items more often than with individual items (and 99% of all realistic programs do), then it is a huge improvement to have built-in support for optionally present things. In the ideal case you never have to check for null or presence.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • 26
    Aside from collections/streams, an Optional is also great to make APIs more self-documenting, e.g. `Employee getEmployee()` vs `Optional getEmployee()`. While technically both could return `null`, one of them is far more likely to cause trouble. – amon Jan 18 '18 at 15:01
  • 1
    @kilian Foth can you please given one example with collection ? – user3198603 Jan 18 '18 at 16:14
  • 1
    A `Stream>` doesn't do anything special with `Optional`. Can you give an example to illustrate what you are talking about? – erickson Jan 18 '18 at 19:26
  • 16
    There's plenty of value in an optional of a single value. Being able to `.map()` without having to check for null all along the way is great. E.g., `Optional.of(service).map(Service::getEmployee).map(Employee::getId).ifPresent(this::submitIdForReview);`. – Joshua Taylor Jan 18 '18 at 19:41
  • 9
    I disagree with your initial comment of no added value. Optional provides context to your code. A quick glance can tell you correct to correctness of a function and all cases are covered. Whereas with null check, should you null check every single Reference type on every single method? A similar case can be applied to Classes and public/private modifiers; they add zero value to your code, right? – ArTs Jan 19 '18 at 04:18
  • @JoshuaTaylor I wish Java had adopted C#'s .? operator instead of that ugly syntax. – user253751 Jan 19 '18 at 05:01
  • 3
    @JoshuaTaylor Honestly, compared to *that* expression, I prefer the old-fashioned check for `null`. – Kilian Foth Jan 19 '18 at 09:41
  • @KilianFoth Different people can have different preferences, for sure. If it's written on one line, I agree, it's horrible. If it's got line wrapping with one map, flatMap, filter, ifPresent, etc., per line, I find it much more manageable. The other advantage (or disadvantage, depending on the details), is that it reduces branching within methods. While the underlying implementation of `.map(...)`, etc., have to be checking for the nulls, *this* code is a non-branching chain. That aids in test coverage, and perhaps in cache locality, though the fact that they're all method calls to... – Joshua Taylor Jan 19 '18 at 12:41
  • @KilianFoth ...different objects might mean that that's not particularly helpful or important. (Plus that's definitely a micro-optimization that we shouldn't worry about until it's a problem.) But the "pipeline" of the dataflow *does* help in being able to test things. If it's just a chain of method references, then all you need to check is that you've got (i) the right method references in the right order, and that (ii) the referenced methods behave correctly. – Joshua Taylor Jan 19 '18 at 12:43
  • 2
    Another big advantage of Optional even with single values is that you cannot forget to check for null when you should do. Otherwise it is very easy to forget the check and end up with a NullPointerException... – Sean Burton Jan 19 '18 at 15:35
  • 1
    @KilianFoth when you said `There is huge added value in having a Collection of such things. All the streaming methods introduced to replace old-style low-level loops understand Optional things and do the right thing about them (not processing them or ultimately returning another unset Optional)` . Can you please elaborate it ? do you mean `items.stream().filter(s->s.contains("B"))` ,if s is optional object then filter method internally check if it is null or not with `ifPresent()` . something like this ? – user3198603 Jan 21 '18 at 13:34
  • Can someone please answer the doubt on my last comment ? – user3198603 Jan 25 '18 at 01:54
  • 1
    This answer does not look right. As Amon also said `A Stream> doesn't do anything special with Optional` . So statement in this answer `There is huge added value in having a Collection of such things. All the streaming methods introduced to replace old-style low-level loops understand Optional things and do the right thing about them (not processing them or ultimately returning another unset Optional)` does not look right . I have already asked the author to elaborate it multiple times but no help – user3198603 Jan 27 '18 at 11:59
  • “There is next to no added value in having an Optional of a single value.”—But that is exactly what `Optional` is: an optional instance of `T` (single value). I don’t understand what you’re talking about. – Guildenstern Jul 09 '20 at 18:27
19

As long as you use Optional just like a fancy API for isNotNull(), then yes, you won't find any differences with just checking for null.

Things you should NOT use Optional for

Using Optional just to check for the presence of a value is Bad Code™:

// BAD CODE ™ --  just check getEmployee() != null  
Optional<Employee> employeeOptional =  Optional.ofNullable(employeeService.getEmployee());  
if(employeeOptional.isPresent()) {  
    Employee employee = employeeOptional.get();  
    System.out.println(employee.getId());
}  

Things you should use Optional for

Avoid a value not being present, by producing one when necessary when using null-returning API methods:

Employee employee = Optional.ofNullable(employeeService.getEmployee())
                            .orElseGet(Employee::new);
System.out.println(employee.getId());

Or, if creating a new Employee is too costly:

Optional<Employee> employee = Optional.ofNullable(employeeService.getEmployee());
System.out.println(employee.map(Employee::getId).orElse("No employee found"));

Also, making everybody aware that your methods might not return a value (if, for whatever reason, you cannot return a default value like above):

// Your code without Optional
public Employee getEmployee() {
    return someCondition ? null : someEmployee;
}
// Someone else's code
Employee employee = getEmployee(); // compiler doesn't complain
// employee.get...() -> NPE awaiting to happen, devs criticizing your code

// Your code with Optional
public Optional<Employee> getEmployee() {
    return someCondition ? Optional.empty() : Optional.of(someEmployee);
}
// Someone else's code
Employee employee = getEmployee(); // compiler complains about incompatible types
// Now devs either declare Optional<Employee>, or just do employee = getEmployee().get(), but do so consciously -- not your fault.

And finally, there's all the stream-like methods that have already been explained in other answers, though those are not really you deciding to use Optional but making use of the Optional provided by someone else.

walen
  • 345
  • 2
  • 9
  • The problem of using Optional to indicate a method might not return a value is, that it often leads to the same code as in your first example of bad code by people who use that method. There should be some other considerations other than just "method might not return a value", otherwise you end up with replacing every null with Optional. – kapex Jan 19 '18 at 09:25
  • 1
    @Kapep Indeed. We had this debate over at /r/java, and [I said](https://www.reddit.com/r/java/comments/7oykna/java_8_optional_use_cases/dsf6cfy/): _«I see `Optional` as a missed opportunity. "Here, have this new `Optional` thing I made so you are forced to check for null values. Oh, and by the way... I added a `get()` method to it so you don't actually have to check for anything ;) ;)". (...) `Optional` is nice as a "THIS MIGHT BE NULL" neon sign, but the bare existence of its `get()` method cripples it»_. But OP was asking for reasons to use `Optional`, not reasons against it or design flaws. – walen Jan 19 '18 at 09:40
  • 1
    `Employee employee = Optional.ofNullable(employeeService.getEmployee());` In your third snippet, shouldn't the type be `Optional`? – Charlie Harding Jan 19 '18 at 12:55
  • @CharlieHarding Yes, it should. Missed it while moving code around in the answer box. Thanks! – walen Jan 19 '18 at 12:58
  • @walen `Optional` would be even more crippled without `get`... – user253751 Jan 20 '18 at 11:27
  • 2
    @immibis In what way crippled? The main reason to use `get` is if you have to interact with some legacy code. I can't think of many valid reasons in new code to ever use `get`. That said when interacting with legacy code there's often no alternative, but still walen has a point that the existence of `get` causes beginners to write bad code. – Voo Jan 20 '18 at 12:48
  • @Voo I know your point is that you should use `ifPresent` and then you can't accidentally `get` an absent value, but then you have logic errors caused by `ifPresent`, which I'm not convinced outweigh the logic errors caused by accidentally getting absent values. (Do you think having an `int.ifNonZeroDivide` and no divide operator would solve division-by-zero errors forever?) – user253751 Jan 21 '18 at 08:41
  • 1
    @immibis Forcing people to explicitly deal with "what happens if divisor is 0" certainly would reduce those errors and make bugs more obvious. But that's not my point here: When writing new code I simply can't think of a single example where you'd want to use get. So if ignoring legacy code, what code do you have in mind where not having a get would cripple the interface? – Voo Jan 22 '18 at 07:25
  • Whether changing to ifPresent would actually reduce bugs is a different question, but it's not really crippling if the only thing it does is not improve things. – Voo Jan 22 '18 at 07:28
  • @Voo It certainly would eliminate divide-by-zero errors (because you can't divide by zero), but it would introduce a new category of did-the-wrong-thing-instead-of-dividing-by-zero errors. – user253751 Jan 22 '18 at 21:29
  • @Voo You can't think of one case where you might have an `Optional` that you know is present and want to get a value out of it? When they take your suggestion and make `Map.get` return an Optional you'll be doing a lot of `myMap.get(...).get()` if you know the entry is in the map. – user253751 Jan 22 '18 at 21:31
  • @immibis "an `Optional` that you know is there" is almost an oxymoron, a situation where maybe you shouldn't be using `Optional` at all; an edge case not worth the trouble of `get()`. Now if `get()` forced you to catch NSEE, that'd be a totally different thing. But right now, if the point of `Optional` is to make null values safer by wrapping them, then having a method that throws unchecked runtime exceptions when trying to get said null values makes absolutely zero sense, and leads to the same problems that we had with NPE, only worse because of the false sense of security (because Optional). – walen Jan 22 '18 at 23:12
  • @walen see my Map example – user253751 Jan 23 '18 at 00:05
  • 1
    @immibis I didn't mention `Map` once and I'm not aware of anyone making such a drastic non-backwards compatible change, so sure you could intentionally design bad APIs with `Optional` - not sure what that proves. An `Optional` would make sense for some `tryGet` method though. – Voo Jan 25 '18 at 09:42
  • 2
    @Voo `Map` is an example everyone is familiar with. Of course they can't make `Map.get` return an Optional because of backwards compatibility, but you can easily imagine another Map-like class that wouldn't have such compatibility constraints. Say `class UserRepository {Optional getUserDetails(int userID) {...}}`. Now you want to get the logged in user's details, and you know they exist because they managed to log in and your system never deletes users. So you do `theUserRepository.getUserDetails(loggedInUserID).get()`. – user253751 Jan 25 '18 at 21:29
  • @immibis If your use case is "people generally expect thing to exist and if it doesn't that's a programming error" then absolutely don't use an optional, the idea is not that we replace every normal class with an optional. It's simply a question of good design and intuition when to use what. That can be difficult - the majority of all APIs out there are suboptimal - but optional doesn't make this that much harder. – Voo Jan 25 '18 at 22:10
  • @Voo You may be surprised how often people would say that such methods (including `Map.get`) should always return an `Optional`, even if you are not one of them. – user253751 Jan 25 '18 at 22:14
  • @immibis Really? In what language? Or do you mean some random people off the street with no actual design experience? All language with optionals that I can think of make the default access for dictionaries throw an exception or something similar - see e.g. Scala for the obvious example in the Java environment. They usually offer an additional method that does return an optional, but that is perfectly legitimate and quite useful. – Voo Jan 25 '18 at 22:19
  • @immibis `NoSuchElementException` because somebody deleted the user by hand / a recent change caused `loggedInUserID` to be wrong / a corrupted index caused the database to return `null` / ... – walen Jan 25 '18 at 22:22
  • @walen I don't see that as a problem. It's no different from getting a NullPointerException because something that should have never been null was null, or getting an ArithmeticException because something that should never have been zero was zero. Bugs are bad, but so is going through excessive contortions to avoid them - if someone edits the database to say this user has 5000 karma and 0 posts and the application tries to display the average karma per post - that's that person's fault for putting invalid data in the database. – user253751 Jan 25 '18 at 22:32
  • Also I'll point out that not allowing `get()` means you need two almost-identical variations of those functions. And neither one can be written efficiently in terms of the other (who here hates using exceptions for control flow?). With `Optional.get()` you have `V get(K key) throws NoSuchElementException {return tryGet(key).get();}` – user253751 Jan 25 '18 at 22:32
  • @immibis _"It's no different from getting a NullPointerException"_ -- which is **the whole effing point**. The existence of `get()` lets people write code just as null-UNsafe as always, but now they think they are safe "because `Optional`". This wouldn't happen if e.g. `get()` forced you to catch the exception. – walen Jan 25 '18 at 22:44
  • @walen The existence of `/` lets people write code that is zero-unsafe. Should the language fix this? Having to type `.get()` reminds the programmer "hey, this thing is likely to be null so double check your assumptions" which in my view is enough of an improvement without being an inconvenience. – user253751 Jan 25 '18 at 22:46
  • @immibis The existence of programming languages lets people write unsafe code. Still not a reason to let new features be as unsafe as older ones. – walen Jan 25 '18 at 22:57
  • Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/72253/discussion-between-immibis-and-walen). – user253751 Jan 25 '18 at 23:03
  • `.orElseGet(Employee::new)` is probably not a sensible thing to do here! – user253751 Mar 06 '20 at 12:38
  • @user253751 In some cases it might not be, in some others it may be OK to do so. E.g. for upsert operations it is completely sensible to use the existing Employee if it exists, or else a new one if it does not. Anyway, it's just an example on how to provide a default value. – walen Mar 06 '20 at 12:59
12

The key point for me in using Optional is keep clear when developer need check if function return value or not.

//service 1
Optional<Employee> employeeOptional = employeeService.getEmployee();
if(employeeOptional.isPresent()){
    Employee employeeOptional= employeeOptional.get();
    System.out.println(employee.getId());
}

//service 2
Employee employee = employeeService.getEmployeeXTPO();
System.out.println(employee.getId());
Rodrigo Menezes
  • 340
  • 1
  • 4
  • 10
  • 9
    It baffles me that all the nifty functional stuff with optionals, although really nice to have, is considered more important than this point right here. Before java 8, you had to dig through Javadoc to know if you had to null-check the response from a call. Post java 8, you know from the return type if you can get "nothing" back or not. – Buhb Jan 19 '18 at 07:02
  • 3
    Well, there is the "old code" problem where things still could return null... but yes, being able to tell from the signature is great. – Haakon Løtveit Jan 19 '18 at 13:50
  • 5
    Yeah it certainly works better in languages where optional is **the only way** to express that a value might be missing, i.e. languages without null ptr. – dureuill Jan 20 '18 at 10:03
8

Your main mistake is that you're still thinking in more procedural terms. This is not meant as a criticism of you as a person, it's merely an observation. Thinking in more functional terms comes with time and practice, and therefore the methods isPresent and get look like the most obvious correct things to call for you. Your secondary minor mistake is creating your Optional inside your method. The Optional is meant to help document that something may or may not return a value. You might get nothing.

This has led you do write perfectly legible code that seems perfectly reasonable, but you were seduced by the vile twin temptresses that are get and isPresent.

Of course the question quickly becomes "why are isPresent and get even there?"

A thing that many people here miss is that isPresent() is that it's not something that is made for new code written by people fully on board with how gosh-darn useful lambdas are, and who like the functional thing.

It does however, give us a few (two) good, great, glamourous(?) benefits:

  • It eases transition of legacy code to use the new features.
  • It eases the learning curves of Optional.

The first one is rather simple.

Imagine you have an API that looked like this:

public interface SnickersCounter {
  /** 
   * Provides a proper count of how many snickers have been consumed in total.
   */
  public SnickersCount howManySnickersHaveBeenEaten();

  /**
    * returns the last snickers eaten.<br>
    * If no snickers have been eaten null is returned for contrived reasons.
    */
  public Snickers lastConsumedSnickers();
}

And you had a legacy class using this as such (fill in the blanks):

Snickers lastSnickers = snickersCounter.lastConsumedSnickers();
if(null == lastSnickers) {
  throw new NoSuchSnickersException();
}
else {
  consumer.giveDiabetes(lastSnickers);
}

A contrived example to be sure. But bear with me here.

Java 8 has now launched and we're scrambling to get aboard. So one of the things we do is that we want to replace our old interface with something that returns Optional. Why? Because as someone else has already graciously mentioned: This takes the guesswork out of whether or not something can be null This has already been pointed out by others. But now we have a problem. Imagine we have (excuse me while I hit alt+F7 on an innocent method), 46 places where this method is called in well tested legacy code that does an excellent job otherwise. Now you have to update all of these.

THIS is where isPresent shines.

Because now: Snickers lastSnickers = snickersCounter.lastConsumedSnickers(); if(null == lastSnickers) { throw new NoSuchSnickersException(); } else { consumer.giveDiabetes(lastSnickers); }

becomes:

Optional<Snickers> lastSnickers = snickersCounter.lastConsumedSnickers();
if(!lastSnickers.isPresent()) {
  throw new NoSuchSnickersException();
}
else {
  consumer.giveDiabetes(lastSnickers.get());
}

And this is a simple change you can give the new junior: He can do something useful, and he'll get to explore the codebase at the same time. win-win. After all, something akin to this pattern is pretty widespread. And now you don't have to rewrite the code to use lambdas or anything. (In this particular case it would be trivial, but I leave thinking up examples where it would be difficult as an exercise to the reader.)

Notice that this means that the way you did it is essentially a way to deal with legacy code without doing costly rewrites. So what about new code?

Well, in your case, where you just want to print something out, you'd simply do:

snickersCounter.lastConsumedSnickers().ifPresent(System.out::println);

Which is pretty simple, and perfectly clear. The point that's slowly bubbling up to the surface then, is that there exist use cases for get() and isPresent(). They're there for letting you modify existing code mechanically to use the newer types without thinking too much about it. What you're doing is therefore misguided in the following ways:

  • You're calling a method that may return null. The correct idea would be that the method returns null.
  • You're using the legacy bandaid methods to deal with this optional, instead of using the tasty new methods that contain lambda fanciness.

If you do want to use Optional as a simple null-safety check, what you should have done is simply this:

new Optional.ofNullable(employeeServive.getEmployee())
    .map(Employee::getId)
    .ifPresent(System.out::println);

Of course, the good looking version of this looks like:

employeeService.getEmployee()
    .map(Employee::getId)
    .ifPresent(System.out::println);

By the way, while it is by no means required, I do recommend using a new line per operation, so that it's easier to read. Easy to read and understand beats conciseness any day of the week.

This is of course a very simple example where it's easy to understand everything that we're trying to do. It's not always this simple in real life. But notice how in this example, what we're expressing is our intentions. We want to GET the employee, GET his ID, and if possible, print it. This is the second big win with Optional. It allows us to create clearer code. I also do think that doing things like making a method that does a bunch of stuff so that you can feed it to a map is in general a good idea.

Haakon Løtveit
  • 309
  • 1
  • 3
  • 3
    The problem here is that you try to make it sound like these functional versions are just as readable as the older versions, in one case even saying an example was "perfectly clear." That is not the case. That example was clear, but not *perfectly* so, as it requires more thought. `map(x).ifPresent(f)` simply does not make the same kind of intuitive sense that `if(o != null) f(x)` does. The `if` version is perfectly clear. This is another case of people jumping on a popular band-wagon, _*not*_ a case of real progress. – Aaron Jan 19 '18 at 20:36
  • 2
    Note that I am not saying there is zero benefit in the use of functional programming or of `Optional`. I was referring only to the use of optional in this specific case, and more-so to the readability, since multiple people are acting like this format is equally or more readable than `if`. – Aaron Jan 19 '18 at 20:40
  • 1
    It depends on our notions of clarity. While you do make very good points, I would like to also point out that to many people lambdas were new and strange at some point. I would dare wager an internet cookie that quite a few people say isPresent and get and felt delightful relief: They could now get on with updating the legacy code and then learn the lambda syntax later. On the other hand, people with Lisp, Haskell or similar language experience were probably delighted that they could now apply familiar patterns to their problems in java... – Haakon Løtveit Jan 22 '18 at 12:00
  • 1
    @Aaron - clarity isn't the point of Optional types. I personally find that they don't *decrease* clarity, and there are some cases (albeit not this case) where they do increase it, but the *key benefit* is that (as long as you avoid using `isPresent` and `get` as much as is realistically possible) they improve *safety*. It is harder to write incorrect code that fails to check for absence of a value if the type is `Optional` rather than using a nullable `Whatever`. – Jules Mar 05 '18 at 16:52
  • Even if you immediately take the values out, *as long as you don't use `get`*, you're forced to choose what to do when a missing value occurs: you would either use `orElse()` (or `orElseGet()`) to supply a default, or `orElseThrow()` to throw a *chosen exception that documents the nature of the problem*, which is better than a generic NPE that is meaningless until you dig into the stack trace. – Jules Mar 05 '18 at 16:52
  • @Aaron “simply does not make as much...”—This is just a bare assertion where “simply” works as a rhetorical device to try to pass off the assertion as an authoritative fact. This is a subjective judgement and we can’t decisively say that it is more or less readable. – Guildenstern Jul 09 '20 at 18:34
  • True - readability is somewhat subjective. – nsandersen Nov 12 '21 at 09:25
0

I don't see benefit in Style 2. You still need to realize that you need the null check and now it's even larger, thus less readable.

Better style would be, if employeeServive.getEmployee() would return Optional and then the code would become:

Optional<Employee> employeeOptional = employeeServive.getEmployee();
if(employeeOptional.isPresent()){
    Employee employee= employeeOptional.get();
    System.out.println(employee.getId());
}

This way you cannot callemployeeServive.getEmployee().getId() and you notice that you should handle optional valoue somehow. And if in your whole codebase methods never return null (or almost never with well known to your team exceptions to this rule), it will increase safety against NPE.

user470365
  • 1,229
  • 6
  • 8
  • Even if if employeeService.getEmployee() return Optional then i do not see any value addition. I think what you mean here is it will force the developer to check for null pointer check with `if(employeeOptional.isPresent())` ? – user3198603 Jan 18 '18 at 15:15
  • 13
    Optional becomes a pointless complication the moment you use `isPresent()`. We use optional so we don't have to test. – candied_orange Jan 18 '18 at 15:53
  • 2
    Like others have said, do not use `isPresent()`. This is the wrong approach to optionals. Use `map` or `flatMap` instead. – Andres F. Jan 18 '18 at 19:45
  • 1
    @CandiedOrange And yet the top-voted answer (saying to use `ifPresent`) is just another way to test. – user253751 Jan 19 '18 at 04:44
  • @immibis you'll notice that top-voted answer isn't testing the return value from `ifPresent()`. – candied_orange Jan 19 '18 at 04:47
  • 1
    @CandiedOrange `ifPresent(x)` is just as much of a test as `if(present) {x}`. The return value is irrelevant. – user253751 Jan 19 '18 at 04:58
  • @immibis one way pulls details towards you where you can obsess over controlling them. The other way pushes details away so you can make other things deal with them. – candied_orange Jan 19 '18 at 05:12
  • 1
    @CandiedOrange Nonetheless, you originally said "so we don't have to test." You can't say "so we don't have to test" when you are, in fact, still testing... which you are. – Aaron Jan 19 '18 at 20:44
  • @Aaron well yes, semantically, which is unfortunate. If it had been called `doit()` the test would have no visibility at this level of abstraction. There are other ways of using optional that achieve this. The improvement in this case is merely structural. – candied_orange Jan 19 '18 at 23:41
  • @CandiedOrange The only difference is that `ifPresent` calls `get` for you. If you weren't going to use the result of `get` then there is no relevant difference. There is no "pulling details towards you". There is no difference in abstraction level. (Now, if you *did* need the value then there is a difference, but you mentioned the test specifically, not the value) – user253751 Jan 20 '18 at 11:25
  • @immibis conceptually, `Optional::ifPresent` is just an alias for `Optional::map`, which is required because `void` breaks all the rules – Caleth Mar 05 '18 at 16:09
  • @Caleth I would bet that if you talked about using `someOptional.map((v) -> {System.out.println(v); return SOME_DUMMY_VALUE;})` you would get some rhetoric about how `map` expressions shouldn't have side effects. – user253751 Mar 05 '18 at 23:30
0

I think the biggest benefit is that if your method signature returns an Employee you don't check for null. You know by that signature that it's guaranteed to return an employee. You don't need to handle a failure case. With an optional you know you do.

In code I've seen there are null checks that will never fail since people don't want to trace through the code to determine if a null is possible, so they throw null checks everywhere defensively. This makes the code a bit slower, but more importantly it makes the code much noisier.

However, for this to work, you need to apply this pattern consistently.

Sled
  • 1,868
  • 2
  • 17
  • 24
  • 1
    The simpler way to achieve this is using `@Nullable` and `@ReturnValuesAreNonnullByDefault` together with static analysis. – maaartinus Jan 19 '18 at 08:07
  • 1
    @maaartinus Adding additional tooling in the form of static analysis and documentation in the decorator annotations, is _simpler_ then compile time API support? – Sled Jan 23 '18 at 04:14
  • Adding additional tooling indeed is sort of simpler, as it requires no code changes. Moreover, you want to have anyway as it catches other bugs, too. +++ I wrote a trivial script adding `package-info.java` with `@ReturnValuesAreNonnullByDefault` to all my packages, so all I have to do is to add `@Nullable` where you have to switch to `Optional`. This means fewer chars, no added garbage and no runtime-overhead. I don't have to look up the documentation as it shows when hovering over the method. The static analysis tool catches mistakes and later nullability changes. – maaartinus Jan 23 '18 at 19:45
  • My biggest concern with `Optional` is that it adds an alternative way of dealing with nullability. If it was in Java since the very beginning, it may be fine, but now it's just a pain. Going the Kotlin way would be IMHO much better. +++ Note that `List<@Nullable String>` is still a list of strings, while `List>` is not. – maaartinus Jan 23 '18 at 19:47
0

My answer is: Just don't. At least think twice about whether it's really an improvement.

An expression (taken from another answer) like

Optional.of(employeeService)                 // definitely have the service
        .map(EmployeeService::getEmployee)   // getEmployee() might return null
        .map(Employee::getId)                // get ID from employee if there is one
        .ifPresent(System.out::println);     // and if there is an ID, print it

seems to be the right functional way of dealing with originally nullable returning methods. It looks nice, it never throws and it never makes you think about handling the "absent" case.

It looks better than the old-style way

Employee employee = employeeService.getEmployee();
if (employee != null) {
    ID id = employee.getId();
    if (id != null) {
        System.out.println(id);
    }
}

which makes it obvious that there may be else clauses.

The functional style

  • looks completely different (and is unreadable for people unused to it)
  • is a pain to debug
  • can't be easily extended for handling the "absent" case (orElse isn't always sufficient)
  • misleads to ignoring the "absent" case

In no case it should be used as a panacea. If it was universally applicable, then the semantics of the . operator should be replaced by the semantics of the ?. operator, the NPE forgotten and all problems ignored.


While being a big Java fan, I must say that the functional style is just a poor Java man's substitute of the statement

employeeService
.getEmployee()
?.getId()
?.apply(id => System.out.println(id));

well known from other languages. Do we agree that this statement

  • is not (really) functional?
  • is very similar to the old-style code, just much simpler?
  • is much more readable than the functional style?
  • is debugger-friendly?
maaartinus
  • 2,633
  • 1
  • 21
  • 29
  • 2
    You seem to be describing C#'s *implementation* of this concept with a language feature as entirely different to Java's *implementation* with a core library class. They look the same to me – Caleth Mar 05 '18 at 09:31
  • @Caleth No way. We could speak about the "concept of simplifying a null-safe evaluation", but I wouldn't call it a "concept". We can say that Java implements the same thing using a core library class, but it's just a mess. Just start with `employeeService.getEmployee().getId().apply(id => System.out.println(id));` and make it null-safe once using the Safe navigation operator and once using the `Optional`. Sure, we could call it an "implementation detail", but implementation matters. There are cases when lamdas make code simpler and nicer, but here it's just an ugly abuse. – maaartinus Mar 05 '18 at 12:53
  • 1
    Library class: `Optional.of(employeeService).map(EmployeeService::getEmployee).map(Employee::getId).ifPresent(System.out::println);` Language feature: `employeeService.getEmployee()?.getId()?.apply(id => System.out.println(id));`. Two syntaxes, but they end up looking **very similar** to me. And the "concept" is `Optional` aka `Nullable` aka `Maybe` – Caleth Mar 05 '18 at 13:00
  • I don't understand why you think one of those is "ugly abuse", and the other good. I could understand you thinking *both* "ugly abuse" or *both* good. – Caleth Mar 05 '18 at 13:07
  • @Caleth One difference is that instead of adding two question marks, you have to rewrite it all. The problem is not that the Language feature and the Library class codes differ a lot. That's OK. The problem is that the original code and the Library class codes differ a lot. Same idea, different code. That's too bad. `+++` What gets abused are lamdas for handling nullability. Lamdas are fine, but `Optional` is not. Nullability simply needs a proper language support like in Kotlin. Another problem: `List>` is unrelated to `List`, where we'd need them to be related. – maaartinus Mar 05 '18 at 13:22
  • @Caleth ... Now when we've got `Optional`, we won't get a proper nullability control, we won't get language support for lists of nullable/non-null/maybe-null strings. This is something a library can't solve. `+++` Moreover, now we have both `Optional` and `null` and tons of libraries using `null`. `Optional` isn't universal, it's not meant to be used for fields, it's not Serializable. It's pain in the a. – maaartinus Mar 05 '18 at 13:26
  • "This is something a library can't solve" -> tell that to Haskell's `Data.List`, .Net's `Enumerable`, C++'s `boost::range` etc – Caleth Mar 05 '18 at 13:29
  • @Caleth OK, then how would a Java library look like which makes the following code compile: List stringList = ...; List> oList = stringList;`? Or something like this in Java. – maaartinus Mar 05 '18 at 13:37
  • 1
    `List stringList = oList.stream().filter(Optional::isPresent).collect(Collectors::toList);`. But I would prefer that as a `Stream> -> Stream` conversion – Caleth Mar 05 '18 at 13:44
  • 1
    Oops, that's the wrong way round. `List> oList = sList.stream().map(Optional::of).collect(Collectors::toList);` – Caleth Mar 05 '18 at 13:47
  • @Caleth What you wrote is a new List creation copying the whole content where I wanted a cast. Something like `List<@NotNull String> stringList = ...; List<@Nullable String> oList = stringList;`. Such a cast may be unsafe, just like with `List` vs. `List`, however copying is not an alternative - Java List's are mutable and identity matters. – maaartinus Mar 05 '18 at 13:53
  • 1
    I'm **glad** the rules of Java **forbid** that – Caleth Mar 05 '18 at 13:56
  • @Caleth It's good that Java forbids mixing *unrelated* types. It's bad that we have `Optional` as a type *unrelated* to `X`. We could have a null-safe type system like [Ceylon](https://en.wikipedia.org/wiki/Ceylon_(programming_language)#Null_safety) or like [described in the third bullet of this wonderful rant](https://groups.google.com/d/msg/project-lombok/ROx9rGRb6lI/EF0lk8F0N10J), but what we got instead, is `Optional`, which IMHO is more a problem than a solution. – maaartinus Mar 05 '18 at 15:41
0

What would happen if "employeeService" is null itself? In both the code snippet, the null pointer exception will be raised.

It can be handled without using optional as:

if(employeeServive != null) {
    Employee employee = employeeServive.getEmployee();
    if(employee!=null){
        System.out.println(employee.getId());
    }
}

But as you can see there are multiple if checks and it can be grown to long hierarchy with a long nested structure like employeeService.getEmployee().getDepartment().getName()....getXXX().getYYY() etc.

To handle this type of scenarios, we can use Optional class as:

Optional.ofNullable(employeeService)
     .map(service -> service.getEmployee())
     .map(employee -> employee.getId())
     .ifPresent(System.out::println);

And it can handle any size of nesting.

To learn all about Java Optional, read my article for this topic : Optional in Java 8

-2

Optional is a concept (a higher order type) coming from functional programming. Using it does away with nested null checking and saves you from the pyramid of doom.

Petras Purlys
  • 368
  • 1
  • 9