Skip to content

Conversation

@mperry
Copy link
Contributor

@mperry mperry commented Jun 18, 2016

No description provided.

@mperry
Copy link
Contributor Author

mperry commented Jun 18, 2016

@jbgi Can you review this please? This addresses #261

final A y = ys.head();

if (o.isLessThan(x, y)) {
if (o.isLessThan(x, y) || o.eq(x, y)) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding a isLessThanOrEqualTo to Ord would be useful to avoid doing the comparison twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I have pushed this change.

Stream<Integer> s = ft.toStream();
System.out.println(s.toList());

out.println(midPriorityQueue());
Copy link
Member

Choose a reason for hiding this comment

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

missing asserts?
(same for split)
Also commented code and println calls should probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was playing with the code to understand what was happening. Removed.

@jbgi
Copy link
Member

jbgi commented Jun 19, 2016

@mperry I opened a PR on your branch with some other suggestions: mperry#2

@mperry
Copy link
Contributor Author

mperry commented Jun 19, 2016

I have merged these and addressed each of these issues.

Do you think it is ready to merge into the Functional Java master branch?

I am too close to the code now and find it hard to spot the errors/improvements.

@jbgi
Copy link
Member

jbgi commented Jun 19, 2016

@mperry yep, look good to me!

@jbgi jbgi merged commit 4a7de0c into functionaljava:master Jun 19, 2016
@mperry mperry added this to the v4.6 milestone Jul 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants