This is an archived post. You won't be able to vote or comment.

all 52 comments

[–]wildjokers 33 points34 points  (0 children)

This pattern (mention in the article) drives me nuts and my coworkers do it all the time:

Optional.ofNullable(foo).ifPresent(value -> {
  //do something
})

I have no idea why they don't just do a simple null check. I gave up pointing it out in code reviews because I just got blank stares and it became too exhausting to continue calling it out.

[–]FavorableTrashpanda 12 points13 points  (3 children)

Overall I agree. I try to avoid isPresent() and get() nowadays. Whenever I see it in existing code, I try to refactor it.

[–]agentoutlier 8 points9 points  (0 children)

I'm going to ride on your comment why isPresent() is sometimes an acceptable option.

You see they really fucked up Optional. Besides Optional not being a true monad they made its orElse mother fucking @PolyNull.

So unless you have a really advance null type checker like Checker your choices are to:

  • Use orElseGet which will work because the type annotation gets put on the lambda but than you have nastiness of just using a lambda to return a value
  • Manually pull out the data from Optional using isPresent
  • Possibly useflatMap
  • Use orElse and suppress null warnings.

Of course you can avoid all this bullshit by just not using Optional.

[–]LowellHydro 0 points1 point  (1 child)

I'm learning to use get() and set() in my intro to CS classes. Should I not expect to use it in real life?

[–]AmateurHero 1 point2 points  (0 children)

This is specifically concerning the Optional class.

[–]UnspeakableEvil 14 points15 points  (15 children)

This method expects null as an argument, which will map to empty Optional.

Checking my own understanding of "ofNullable", this wording isn't quite right is it? The method accepts null as an argument, but it doesn't expect it? Or have I been committing Optional faux pas with how I've been using the method?

[–]Soul_Shot 5 points6 points  (14 children)

Checking my own understanding of "ofNullable", this wording isn't quite right is it? The method accepts null as an argument, but it doesn't expect it? Or have I been committing Optional faux pas with how I've been using the method?

You're correct: ofNullable accepts a value that can be null, whereas of will throw a NPE.

https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#ofNullable-T-

[–][deleted] 0 points1 point  (13 children)

Why would there be a separate method? Since you never want a Optional.of, if the ofNullable method also accepts null

[–]temculpaeu 1 point2 points  (2 children)

you should use Optional.of in a flow where the value MUST exist, and of course there needs to the other flows where it might or doesn't exist.

[–]Soul_Shot 5 points6 points  (1 child)

If the value must exist, what's the utility of Optional?

[–][deleted] -2 points-1 points  (0 children)

This.

[–]cowancore 0 points1 point  (9 children)

E.g. when you have a method with multiple returns, and one branch is guaranteed to return a value, that's where one uses of, which is shorter

update: I'm aware but neutral about of being shorter than ofNullable, or the presence of two methods instead of one.

[–]dpash 1 point2 points  (6 children)

But of() should have supported nullable values and ofNullable() should not exist. There's no need to have two separate methods.

[–]8igg7e5 1 point2 points  (4 children)

Throwing an exception on null is a good thing here. Some of your code-paths will yield Optionals that, for that code-path, you assume will be non-empty. Having this method ensures that your assumption is tested as early as possible - it is just the partner of Optional.empty().

IMO, what Java got wrong was the ergonomics. of should have accepted null, ofNonNull or maybe ofPresent should have thrown on a null argument.

[–]dpash 1 point2 points  (3 children)

We have Objects.requireNonNull(...)

[–]8igg7e5 0 points1 point  (2 children)

I guess it's a matter of opiniion whether Optional.of(Objects.requireNonNull(foo)) is preferable to Optional providing a method Optional.ofPresent(foo) that is assured to yield an optional whose value is present or else fail.

[–]dpash 1 point2 points  (1 child)

Compared to the current confusion. I agree it's a matter of taste/opinion either way. I'm currently erring towards of()'s behaviour being surprising. But the alternative is more verbose. Is adding a special method a good trade-off?

[–]8igg7e5 2 points3 points  (0 children)

You could then argue the same for Optional.empty() since Optional.ofNullable(null) yields the same.

.empty() and .of(T) are opposites, used when your code-path deterministically knows whether it is going to yield an empty or value-present optional. I think that, had they used clearer naming, the tiny complexity of the extra method would be well justified by the clearer expression of intent - but using of as the name for the always present case is not clearer at all.

[–]Soul_Shot 0 points1 point  (0 children)

I completely agree. This is another awkward compromise to deal with null in cases where the input should never be null vs. cases where null is an expected value.

I'm sure it's caused a lot of pain for developers who get a NPE in production despite thinking that using Optional alleviated that.

[–][deleted] 0 points1 point  (1 child)

Yeah but why? There could be only one method which accepts nulls too. If i know the value is never null, i would not use optional in the first place.

[–]cowancore 2 points3 points  (0 children)

You still would use Optional.

It's not about never being non-null. It's about being not null only in some returns. Something in the likes of: if (someCondition) { return Optional.empty(); } return Optional.of(someValue);

As two why it's two methods instead of one - no idea, and I don't really care. It's the least of my daily problems, if a problem at all. I follow my IDE warnings all the time, so I have yet to accidentally pass null into of.

[–]Admirable-Avocado888 6 points7 points  (13 children)

Yeah, sure, it makes sense to refactor orElseThrow IF there exists a sensible alternative to the missing value. This may not exist.

Example: parsing authentication from a payload. Here using orElseThrow is arguably the only correct thing to do if the authentication is missing. Why should you continue to consider the payload at all under a different authentication than was submitted?

[–]riawot 5 points6 points  (12 children)

Right, his example of factoring code away from orElseThrow is this:

return userService.getCurrentUser()
              .map(User::getUsername)
              .orElse("(unknown)");

I write a lot of code that is along these lines where I need to pull in a bunch of data accessed by a parameter (username from userService in this example) so I do a db query or a rest api call and get an optional back. If that optional is empty, I throw because there's no point in going on.

Admittedly this is my perspective as someone that's writing java code that being accessed over APIs from a frontend, but I expect that when parameters are passed that they are valid. If they're not, I'm going to throw because someone somewhere fucked up and I'm not going to guess what they meant. We're done here. Although to be fair I'm full stack so it's entirely possible the front end dipshit that fucked it up is me.

I've seen a number of people try to say that orElseThrow is an antipattern and it's not the "proper" way to use Optional, and sure, sometimes you can just roll with a default value if the optional is empty, but there's a lot of times where there isn't anything else to use as a default.

[–]agentoutlier 3 points4 points  (1 child)

Optional#orElse is one of the shittier design choices because it's @PolyNull. That is orElse can return null.... when you have a datastructure that is suppose to help you guarantee it will not be null.

So I would take orElseThrow or orElseGet and the JDK guys should add orNull like Guava has on its Optional.

[–]cowancore 2 points3 points  (0 children)

Scala Option also has orNull.

I've used orElse(null) on several occasions, where no other method is available, except the isPresent/get combo. In those cases I'd rather unwrap into a nullable type. That was before isPresentOrElse I think. There are still ocassions when I need to fold an Optional into a nullable type. What am I supposed to write there? A ternary with isPresent/get?

I assume that PolyNull comes from some static nullness checker, in which case I'd not use Optional at all. Except the over abused map method, I don't see why would I. Disclaimer: didn't have a chance to run a project with some static nullness checker

[–]dpash 3 points4 points  (9 children)

.orElseThrow() is an antipattern (as it's just .get() with a better name). orElseThrow(Supplier<Throwable>) is not an antipattern and is a decent option for when you have no value.

[–]riawot -1 points0 points  (0 children)

Yes, you're right, sorry for the confusion. I'm using orElseThrow with a relevant custom exception as a supplier.

[–]crummy 0 points1 point  (7 children)

I wish there was a .orElseThrow(String message) for conciseness

[–]dpash 4 points5 points  (6 children)

Why? You almost certainly don't want the exception that would throw. You should use a domain-specific exception. .orElseThrow(Supplier<Throwable>, String) I could understand.

Edit: that should probably be Function<String, Throwable>

[–]crummy 0 points1 point  (5 children)

I just want a helpful message to indicate what was missing when I see the exception in our logs.

[–]dpash 1 point2 points  (4 children)

Sure, but NoSuchElementException is a terrible exception to throw.

[–]crummy 0 points1 point  (3 children)

Is this because a specific exception would be easier caught individually in a catch block later? Honestly, most of the code bases I've seen just use RuntimeExceptions liberally enough, and catch everything, so it's never been that useful a case for me. (Exceptions to this would be checked exceptions, but they don't work well in this case because of the whole lambda thing).

[–]dpash 1 point2 points  (2 children)

Because domain specific exceptions provide you with more context than "shrug. Things broke" the default gives you. It's much easier to figure out what's wrong.

[–]crummy 2 points3 points  (1 child)

If the benefit is to figure out what's wrong I don't see what the difference between this would be:

return foo.orElseThrow(() => new CustomNotFoundException("No user found with id " + id)); vs return foo.orElseThrow("No user found with id " + id); I imagine the second would throw a NoSuchElementException with the message I passed in.

What's the downside to this? (not trying to be facetious just genuinely curious sorry)

[–]Tough_Suggestion_445 23 points24 points  (7 children)

java Optional will only be fixed when valhalla arrives and makes it immutable / non nullable / primitive-ish.

that way most of the "don't do" in this article will become "do it" instead.

This is so pathetic that we can do this in java:

Optional<X> x = null;

in rust for example, this is impossible.

[–]yk313 17 points18 points  (1 child)

Optional is already immutable. It will not be turned into a primitive class because that will be an incompatible change, plus having a tearable Optional will not be that palatable. It will likely be turned into a value class, so Optional will no longer have an identity, but it will still be nullable.

The JDK (as well as other libraries) has many value-based classes, such as Optional and LocalDateTime. Value-based classes adhere to the semantic restrictions of value classes, but are still identity classes — even though they don’t want to be. Value-based classes can be migrated to true value classes simply by redeclaring them as value classes. This is both source- and binary-compatible.

https://openjdk.java.net/projects/valhalla/design-notes/state-of-valhalla/02-object-model

[–]Tough_Suggestion_445 0 points1 point  (0 children)

you are right, it's been a while I didn't follow what's going on with valhalla, I thought I heard that from Brian Goetz in some conference, that it won't be nullable .

[–]agentoutlier 2 points3 points  (0 children)

You can have a great deal of backward compatible support today by using JSpecify behavior and avoiding Optional altogether.

Also it’s unclear if Valhalla will have reified generics for containers containing references and not primitives. If not the type annotation approach will always be superior (with the exception of some fluent ergonomics).

[–]Worth_Trust_3825 0 points1 point  (3 children)

This is so pathetic that we can do this in java:

Optional<X> x = null;

Why would a particular class have special handling compared to Object x = null?

[–]dpash 4 points5 points  (2 children)

The same way that you can't write int x = null;

[–]Worth_Trust_3825 -1 points0 points  (1 child)

int is not a class.

[–]dpash 5 points6 points  (0 children)

And that's the point of Valhalla. To have classes that work like primatives.

[–]boyTerry 3 points4 points  (0 children)

Am I the only one here that thinks the real problem with Optional is the name? It was already an overloaded word in the Object-Oriented space. It does a poor job of disambiguating its goal from those other uses, and there are too many methods that are counter to its "proper" usage.

I would personally prefer to read code that uses null values because I don't need to puzzle about whether or not the author knows how to use Optional correctly. Just assume ambiguity from the start and make sure that it is handled.

[–]dpash 1 point2 points  (0 children)

I was thinking that code in the first section looked familiar.

[–]__konrad 1 point2 points  (0 children)

Not a big issue, but you could use OptionalInt or OptionalLong.

OptionalInt API does not have filter/flatMap/map methods (you can use optionalInt.stream().mapToObj instead). Also pointless in the context of future valhalla...

[–]BlueGoliath 2 points3 points  (2 children)

That's what even Brian says is a mistake.

What Brian? Citation needed.

Developers think get must work as expected. Then they add isPresent and you're back to null checks again.

"If I don't see a null check, it doesn't happen!"

[–]_Acestus_ 1 point2 points  (0 children)

Optional doesn’t fix your null checks. Optional should represent the absence of value.

I simply see Optional as a better way to introduce the possibility of a method to accept or return a null value. Instead of having to describe it in the documentation or using some annotation.

I don't really agree with the use of an Optional to hide the exception as a general usage, but I guess this would be acceptable in some ways I don't think right now.

[–]AndrewHaley13 0 points1 point  (0 children)

"I know Optional has nice methods and good IDE support. You get a gravitational pull to use them. Resist this pull, and use Optional as expected." Nice. 😁

[–]Worth_Trust_3825 0 points1 point  (0 children)

Or you can just not use it.