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-795] Add PairedIterator to allow iterating over different-types of iterators #412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anantdamle
Copy link

[COLLECTIONS-795] Add a new Iterator to allowing zipping over two iterators of different types

This is different from ZippingIterator which only permits sub-iterators of same type.
Created as #238 was overwhelmed with multiple commits due to inactivity for 2+ years.

@garydgregory can you please consider this proposal

@Claudenw
Copy link
Contributor

@anantdamle I had a professor who stated there are only 3 valid limits in computing: zero, one, and many. Your solution zips two iterators, have you considered who to generalize that to many? Is that possible?

@anantdamle
Copy link
Author

also fixed it in PairedIterableTest.java

@anantdamle
Copy link
Author

@anantdamle I had a professor who stated there are only 3 valid limits in computing: zero, one, and many. Your solution zips two iterators, have you considered who to generalize that to many? Is that possible?

I think Its possible, and I have tried it, with a working prototype, though the interface becomes too complex as the consitutents can be of different kind (classes), so will have to use Object and class type, which in my opinion becomes a bad design.

class MultiZipIterator implements Iterator<List<Pair<Object, Class>>> 

Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

LGTM
After running the tests, there are a number of issues with style. @anantdamle please run mvn on your local branch and you should see a number of style issues as highlighted in the CI test runs.

Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

Plese fix error reported by CI test.

@anantdamle
Copy link
Author

@Claudenw thanks for reviewing this, fixed checkstyle formatting issues.

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.

@anantdamle
I added a few low-level comments.

@anantdamle
Copy link
Author

@garydgregory made changes as requested and rebased on latest master

fix review comments
rebase on master
fix checkstyle error
use single class imports
@anantdamle
Copy link
Author

Gentle bump up for review

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.

3 participants