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

[COLLECTIONS-674] Add drain method to CollectionUtils #91

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented Oct 23, 2019

@garydgregory
Copy link
Member

You should check your tests before committing ;-) as the build fails.

WRT API name, removeRange is reminiscent of java.util.Arrays.copyOfRange(...) so it should be OK: we are removing a range of items specified by input args.

But we can do better than removeByDrain IMO. "ByDrain" is confusing since 'drain' and 'remove' are synonymous.

It feels to me like a removeCount is more precise.

You could reuse .CollectionUtils.checkIndexBounds(int)

You should also clean up your formatting as it seems all over the place, looking at it in GitHub at least.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Thank you!

@garydgregory garydgregory changed the title Add drain method to CollectionUtils [COLLECTIONS-674] Add drain method to CollectionUtils Nov 1, 2019
@garydgregory garydgregory merged commit c423a99 into apache:master Nov 1, 2019
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.

2 participants