Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of Optional (#52) #93

Merged
merged 3 commits into from Apr 23, 2020
Merged

Conversation

gavinking
Copy link
Member

@gavinking gavinking commented Apr 15, 2020

As proposed in #52, this change kicks Optional out of our code base.

Justification:

  • it doesn't really protect you from NPEs because nothing stops you from calling get() on an uninitialized one (unlike other langs, Java doesn't have sum or union types) ... even worse, nothing stops you from assigning null to the type Optional
  • indeed, it actually causes more bugs than it avoids, since Optional is assignable to Object
  • it makes you create extra local variables to assign the result of get() to (unlike Ceylon, Java doesn't have intersection types)
  • its comes with a performance overhead (Java doesn't have value types or union types)
  • it makes method signatures uglier (unlike other langs, Java doesn't have a syntax sugar for it)

Does anybody want to step up and defend this 'orrible thing?

@@ -35,7 +35,7 @@ public void testLoadNull(VertxTestContext testContext) throws Throwable {
session.find( GuineaPig.class, 1 )
.whenComplete( (result, err) -> {
try {
assertThat( result ).isNotNull();
assertThat( result ).isNull();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you squash this in the previous commit, please?

@DavideD
Copy link
Member

DavideD commented Apr 16, 2020

Isn't the idea of Optional to allow a more functional approach to code (it has functions like map, flatMap, ifPresent and so on)?

It's more likely to be the cause of bugs than it is to protect us from bugs.
.thenCompose( v -> queryExecutor().selectLong(selectIdSql, new Object[0], factory) )
.thenApply(Optional::get);
.thenApply( id -> id );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this last thenApply?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it does a widening cast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This wouldn't be necessary if Java had declaration-site variance grumble.)

@gavinking
Copy link
Member Author

gavinking commented Apr 16, 2020

Isn't the idea of Optional to allow a more functional approach to code (it has functions like map, flatMap, ifPresent and so on)?

That's not what "functional" means. (Not for any meaningful definition of "functional" anyhow.)

Look, we have a reasonable-sized codebase that uses Optional throughout. We don't call map() or flatMap() on it anywhere. And we have no reason to do so: the whole reason those operations have those names in ML or Haskell is that there's an abstraction (a type class) called Functor that lets you operate polymorphically over Lists and Maybes and all sorts of other things, letting you write a special sort of highly generic code that is utterly impossible to write in Java (without or without Optional).

So this is just cargo-cult stuff. The only people who think that calling the monomorphic function Optional.flatMap() is making their code more "functional" are people who don't actually know a single thing about functional programming.

@gavinking
Copy link
Member Author

P.S. @DavideD If you like, I can write you a function applyWhenNotNull() that does exactly what Optional.flatMap() does, but that doesn't require you to wrap every one of your objects in a garbage wrapper object.

@gavinking
Copy link
Member Author

The only people who think....

(That comment wasn't aimed at you, of course, @DavideD, more at the people who designed this thing.)

@DavideD
Copy link
Member

DavideD commented Apr 16, 2020

(That comment wasn't aimed at you, of course, @DavideD, more at the people who designed this thing.)

Sure, but even so... was it really helpful as far as comments go?

Anyway, my point is that Optional is what Java provides and some user might find it useful in some circumstances. It's not that I am a big fan of it, I'm mostly trying to understand if users might find it useful to have it in the API.

@DavideD
Copy link
Member

DavideD commented Apr 16, 2020

As far as I can tell most programmers don't like it, so I'm not against removing them from the API. I'm not sure this is the right place to discuss it though, as people won't see it.

@gavinking
Copy link
Member Author

I'm not sure this is the right place to discuss it though, as people won't see it.

Right, that's why I opened #52 a couple of months ago ... to try and get some feedback.

(And we have to make a decision on this ASAP.)

@vietj
Copy link

vietj commented Apr 16, 2020

Optional is evil

@NathanQingyangXu
Copy link

NathanQingyangXu commented Apr 21, 2020

There is a serious drawback for the Optional from JDK. It doesn't implement Serializable for JDK team wanted to avoid the burden to maintain the Serializable backward compatibility.
That being said, Optional is definitively useful in general scenario. Sometimes we wanna differentiate between the following two possibilities:

  • something exists but with null value;
  • something doesn't exist at all;

see https://softwareengineering.stackexchange.com/questions/364211/why-use-optional-in-java-8-instead-of-traditional-null-pointer-checks for some other reasons

No idea about the hibernate-rx relevance, though.

@aguibert
Copy link
Contributor

+1 on removing optional

@gavinking
Copy link
Member Author

Sometimes we wanna differentiate between the following two possibilities:

  • something exists but with null value;
  • something doesn't exist at all;

This is indeed something that arises occasionally (really the only case I know of is caching/memoizing), it's not a distinction we need to make in the API of hibernate-rx.

@gavinking gavinking mentioned this pull request Apr 23, 2020
@DavideD DavideD self-requested a review April 23, 2020 14:56
@gavinking gavinking merged commit a6c8a53 into hibernate:master Apr 23, 2020
@DavideD
Copy link
Member

DavideD commented Apr 23, 2020

NIce, thanks

@gavinking gavinking deleted the kill-optional branch June 15, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants