3

What do you think is better (with arguments, of course):

Optional.ofNullable( userName )
      .ifPresent( nonNullUserName  -> header.setUser( createUser( nonNullUserName ) ) );

or

header.setUser( userName == null ? createUser( userName ) : null );

The method createUser creates xml element and the intent of the whole peace of code is to set it in a SOAP request depending on presence of userName.

The benefits of the first approach I see is the absence of useless operations, the code does one thing and no more. But the second approach lets you save one more line of code thus appearing to be more laconic.

UPDATE: I guess I missed a thing I actually implied and it caused certain misunderstanding. It would be better to provide cleaner examples with explanations if you have some.

Dmitry Senkovich
  • 5,521
  • 8
  • 37
  • 74
  • 7
    They don't even do the same thing. But anyway, Optional is not designed to do that, and is of course a tiny bit less efficient. Optional is designed to be a return type of a method. I would just use `if (userName != null) { header.setUser(createUser(userName)); }` (assuming you want to do what the first snippet does, and not what the second does). – JB Nizet Apr 29 '17 at 14:01
  • 1
    Agreed. The question is flawed as it presents 2 bad ways to do something extremely simple. Talking about saving lines of code is a futile argument when we're dealing with Java, which is a verbose language in many other ways. – Kayaman Apr 29 '17 at 14:03
  • @ JB Nizet Interesting approach. But our code style doesn't let write it in one line so I would say the first approach above and the first answer look cleaner to me. Thanks for the idea – Dmitry Senkovich Apr 29 '17 at 14:11
  • @Kayaman Well, I wouldn't say our two approaches are the only one. That is why I'm here. Can't understand your hate – Dmitry Senkovich Apr 29 '17 at 14:13
  • 2
    The point is not to write it on one line. The comment system doesn't allow multiple lines. The point is to use a simple if block. You shouldn't care if it's on one line or on three. – JB Nizet Apr 29 '17 at 14:14
  • @JB Nizet Of course, agreed. But to my mind the first approach looks cleaner, so I would like to know the opinion of the community. But I agree, there's nothing in count of lines of code – Dmitry Senkovich Apr 29 '17 at 14:16
  • 1
    To have the official point of view of the designers of Optional, working on the JDK, see https://www.youtube.com/watch?v=Ej0sss6cq14 (28th minute). The slide says: *It's generally a bad idea to create an Optional for the specific purpose of chaining methods [...]* – JB Nizet Apr 29 '17 at 14:19
  • in short, `Optional` is designed to avoid `NullPointerException` in jdk-8. – holi-java Apr 29 '17 at 14:20
  • @JB Nizet This sounds like an answer to my question. Could you please anwer it? It might be helpful – Dmitry Senkovich Apr 29 '17 at 14:21
  • 1
    If you think it's hate to point out your question is flawed, then you don't really understand what hate means. – Kayaman Apr 29 '17 at 14:25
  • @holi-java That's not true. Optional doesn't remove null checks, unless you write hard to understand code (like in the question) to explicitly try to use `Optional` in every possible place. – Kayaman Apr 29 '17 at 14:26
  • @Kayaman in the question above, it is. – holi-java Apr 29 '17 at 14:30
  • 1
    @holi-java I'm not talking about the question. I'm talking about your broad statement of "Optional is designed to avoid NullPointerException in jdk-8". It can help avoid null checks, but your statement is not true. – Kayaman Apr 29 '17 at 14:32
  • @Kayaman thanks for your feedback, sir. my comments is to the OP, not to everybody, I'm sorry. – holi-java Apr 29 '17 at 14:43

2 Answers2

17

Optional was designed to be used as a return type for methods that can't always return a value, like for example getFirstElementOfAList(), which can't return anything else if the list is empty: returning null would be less clear than returning an empty Optional, and could mean that the first element is null, and not that there is no first element.

The designers don't intend Optional to be used as a convenience construct to chain methods. See the presentation by Stuart Marks (who works on the JDK), and especially what he says at the 28th minute:

It's generally a bad idea to create an Optional for the specific purpose of chaining methods [...]

You're of course free to disrespect the intended design and the best practices, but your colleagues might also be free to hate you for that :-)

Note that the two snippets you posted don't do the same thing. The first one does nothing if the user name is null, whereas the second one sets the user to null (which might in turn throw an exception or overwrite the previous value)

I would just use (assuming what you want is the first version):

if (userName != null) { 
    header.setUser(createUser(userName)); 
}

which is extremely clear, doesn't abuse Optional, and doesn't create an Optional instance just to chain methods.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • plus one, sir, I agree on, but I want to show the OP that `Optional` can also appearing to be more **laconic** and in my answer I have been described. – holi-java Apr 29 '17 at 14:46
  • 2
    Is there any valid, scientific, or at least empiric argument to *not* use `Optional` to chain methods (as in @holi-java 's answer) besides the intentions of the JDK team? Because, well, I had the intention to be a rockstar, but ended up programming in Java... – fps Apr 29 '17 at 23:15
  • It's less efficient, creates useless objects, and, in this case at least, is less clear than a simple if statement. – JB Nizet Apr 29 '17 at 23:18
  • I get your point, but regarding efficiency, we're talking about a few null checks and a wrapper class => negligible. Regarding useless objects, most people use a framework, i.e. Spring, JSF, etc. That is a few thousand of real, useless objects. Regarding how clear it is, it's just a matter of opinion. The other answer is crystal clear to me. My point is against the statement *someone from Oracle created this to be used this way*. That is no valid reason. – fps Apr 29 '17 at 23:49
  • @JBNizet @FedericoPeraltaSchaffner Hi, but sometime maybe create Optional chain more great where `Optional.map` return in the method like as `getFirstElementOfAList` but you can't control, so you used `Optional.flatMap` in your code. sir, am I right? – holi-java Apr 30 '17 at 02:56
  • @holi-java yes, when used as a return value, `Optional` is very useful. It is expressive, because it clearly states that the value can be present or not. And it is also very handy, because you just use `Optional`'s operations to transform the result and consume it. – fps Apr 30 '17 at 03:13
  • Thanks for referencing my talk. :-) – Stuart Marks May 02 '17 at 00:52
  • 2
    @FedericoPeraltaSchaffner As the guy from Oracle being cited, this isn't about "Somebody from Oracle said not to so you shouldn't do it." Rather, it's about design intent and effectiveness. If something is designed for a purpose, it's likely to be useful for that purpose. It's *less likely* to be useful for purposes for which it wasn't designed. In particular, `Optional` wasn't designed to replace if-statements. In this case, the alternative using `Optional` might be crystal clear. (I don't happen to think so, but reasonable people can disagree on this.) – Stuart Marks May 02 '17 at 00:56
  • 1
    @FedericoPeraltaSchaffner However, when examples get more complicated, such as the cases I cite in my talk (which are originally from Stack Overflow) I think you'll find that using `Optional` this way starts to break down quickly. People can get really clever -- too clever -- at trying to replace control structures with `Optional`. This tends to lead to obscure, hard-to-understand code. Hence I recommend avoiding using `Optional` this way. – Stuart Marks May 02 '17 at 01:02
  • @StuartMarks Hey, thanks for answering my comment. You are giving arguments, and that was precisely my point. The argument shouldnt be "because Stuart Marks said this in a conference", but what you have actually said instead. That was my whole point. – fps May 02 '17 at 01:22
  • @StuartMarks And I agree with you: in this case replacing if with Optional is easy, but in more complex cases using Optional produces code that makes you have nightmares. Same for streams, I think. Again, thank you very much for taking the time to participate and make your point crystal clear. Btw, I have upvoted this answer :) – fps May 02 '17 at 01:32
5

They are different things, one is an Object, another is an operator. you shouldn't comparing between them. the first approach can be simplified to this, which will be more readable & descriablable:

Optional.ofNullable(userName).map(this::createUser).ifPresent(header::setUser);

IF you really want to comparing between them, the only one different is you have been mentioned above. and if you process condition or result more complex in an operator that will result to the expression is too complex to read.

holi-java
  • 29,655
  • 7
  • 72
  • 83
  • Thanks, I really like your suggestion. Gonna see what others think – Dmitry Senkovich Apr 29 '17 at 14:14
  • Well, finally JB Nizet has found the official point of view on the problem. Such constructions as I proposed in the question are not the cases Optional was added for. – Dmitry Senkovich Apr 29 '17 at 14:23
  • @DmitrySenkovich Hi, what I want to tell you is `Optional` can also appearing to be more `laconic` and in my answer I have been described. and I don't suggest you must do with Optional chain. and in my answer I only tell you needn't comparing two different things. – holi-java Apr 29 '17 at 14:49
  • I agree. I also see it as more laconic. No sure I understand you thoughts about comparing it. If I can use two approaches I always can compare it somehow I think. Here I wanted to see the community opinion because we had a dispute in our team. – Dmitry Senkovich Apr 29 '17 at 15:12
  • @DmitrySenkovich well. you should do the first approach in your mind, and after if you find the best one, just replacing the oldest one. if you do everything thinking how and why at first? you will cost many time choosing between them. – holi-java Apr 29 '17 at 15:21
  • Not sure I understand you. Everyone makes mistakes and fixes it if he wants and can do it. Everything evolve, today one approach is better and the day after another one. Programming is all about not doing thing only but evolving also I think – Dmitry Senkovich Apr 29 '17 at 15:25
  • 1
    @DmitrySenkovich To be honest, my English is very bad， If I always thinking how to answer & describe my mind by English, I couldn't answer any question. first I write a broad-brush answer, and then I refactoring my answer, and so on. – holi-java Apr 29 '17 at 15:26
  • Honestly, I fail to see the problem with this approach. Actaully, I find it much better than an imperative `if` construct. – fps Apr 29 '17 at 23:18
  • @FedericoPeraltaSchaffner thanks for your feedback, but they are talking on the performance on this question. – holi-java Apr 30 '17 at 02:46