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

Added Seq.iterateWhilePresent #304

Merged
merged 2 commits into from May 3, 2017
Merged

Conversation

@tlinkowski
Copy link

@tlinkowski tlinkowski commented Apr 26, 2017

Implementation for #303.

@lukaseder
Copy link
Member

@lukaseder lukaseder commented Apr 28, 2017

Thank you very much for your suggestion. I'll review shortly

Loading

@lukaseder lukaseder added this to the Version 0.9.13 milestone Apr 28, 2017
assertEquals(
asList(),
Seq.iterateWhilePresent(null, i -> Optional.empty()).toList()
);
Copy link
Member

@lukaseder lukaseder Apr 28, 2017

Choose a reason for hiding this comment

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

I find this surprising. This should be equal to asList((Object) null). Illustration:

0ba5035

Loading

Copy link
Author

@tlinkowski tlinkowski Apr 28, 2017

Choose a reason for hiding this comment

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

I understand your surprise - the proposed solution is somewhat inconsistent. The reason I let user provide null as seed instead of Optional.empty() is:

  1. Optional<T> is essentialy a wrapper for nullable T
  2. Optional is not supposed to be used as a parameter

I see three solutions:

  1. call Objects.requireNonNull on seed (consequences: the resulting Seq is always non-empty)
  2. change method signature to take Optional<T> seed (consequences: we break the convention)
  3. leave it as it is (consequences: we're inconsistent in the use of Optionals and nullable values)

But what you propose (that null seed returns a Seq with a null value) is - in my opinion - the most incoherent solution of all four. If we adopted this, the only way null could make it into the resulting Seq would be as being its first element (because remaining elements are provided by Optional, and Optional either stores a non-null value or reports it's empty).

Loading

Copy link
Member

@lukaseder lukaseder Apr 28, 2017

Choose a reason for hiding this comment

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

Gah, what a mess :) We wouldn't have that trouble with the predicate-based API. Anyway, I'll think about it...

Loading

Copy link
Author

@tlinkowski tlinkowski Apr 28, 2017

Choose a reason for hiding this comment

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

That's true. Actually, my main objection against Optional is that it's not supposed to be used as field type or parameter type. And I don't truly understand why because current approach creates inconsistencies like the one above. If only Java had Kotlin's null-handling...

Loading

Copy link
Member

@lukaseder lukaseder May 1, 2017

Choose a reason for hiding this comment

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

In this case, it's a return type, not a field/parameter type, so it works along the ideas of the designers, in a way. Even better than Kotlin: Ceylon, with real union types. Your function would return a type like T|EndOfIteration

Which reminds me that Java's checked exceptions are really union types (if you squint really hard). Perhaps, we could abort iteration with a checked exception? ;-) (semi-serious)

Loading

Copy link
Author

@tlinkowski tlinkowski May 1, 2017

Choose a reason for hiding this comment

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

Yeah, checked exceptions - I love them ;)

But what I meant by my objection against Optional is that I'd like to use it as a parameter (Optional<T> seed - solution 2). Actually, when now I look at all these 3 potential solutions, I'm most attracted to this Optional-parameter solution. And we already have Optional as parameter type, e.g. in Seq.seq(Optional). What do you think?

PS. I don't know about Ceylon and its union types but with Kotlin it would be really nice - we'd have:

fun <T> iterateWhilePresent(seed: T?, generator: (T) -> T?): T

Loading

Copy link
Member

@lukaseder lukaseder May 1, 2017

Choose a reason for hiding this comment

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

An Optional is a collection of arity 0..1, which is why it makes sense to wrap it in a Seq. Likewise, JDK 9 will support Optional.stream(). I wish it even implemented Iterable.

In that sense, it might be consistent to allow it as a parameter, but why do that? Why even call iterate when we know the resulting Seq will be empty?I mean, if the seed were Supplier<Optional<T>>, it might make sense (I wouldn't like it either), but with a hard-wired Optional, I think it's better to just not call the method, and value consistency with the existing iterate() method higher

Loading

Copy link
Author

@tlinkowski tlinkowski May 2, 2017

Choose a reason for hiding this comment

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

Well, as to why call iterateWhilePresent when you know that the resulting Seq will be empty - it's for the same reason why you call Seq.seq(Optional) instead of optional.isPresent() ? Seq.of(optional.get()) : Seq.empty(), i.e. for convenience.

But I guess seed usually is non-empty so I agree with you that in this particular case consistency with the existing iterate() is more important than robustness.

Solution 1 (Objects.requireNonNull(seed)) it is, then, right?

Loading

Copy link
Member

@lukaseder lukaseder May 3, 2017

Choose a reason for hiding this comment

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

Yeah, I agree with solution 1. It's also the most forward compatible, if we ever want to do something with this null seed.

Loading

Copy link
Author

@tlinkowski tlinkowski May 3, 2017

Choose a reason for hiding this comment

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

Done.

Loading

@lukaseder lukaseder merged commit c3b3b45 into jOOQ:master May 3, 2017
1 check passed
Loading
@lukaseder
Copy link
Member

@lukaseder lukaseder commented May 3, 2017

Excellent, thank you very much!

Loading

@tlinkowski tlinkowski deleted the iterateWhilePresent branch May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants