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-699 PairingIterator #74

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Fival85
Copy link

@Fival85 Fival85 commented May 22, 2019

Hello again,

like in #73 here is the second try to implement COLLECTIONS-699

Now i use a standalone lightweight dto inside the PairingIterator to return the result.

Is the implementation ok?

[NEW] An iterator for returning pairs of the childs iterators, askes in COLLECTIONS-699
[FIXED] replacing tabs with whitespaces
Instead of a common-lang Pair we use a custom lightweight dto.
Change dependency scope of commons-lang3 back to "test"
@coveralls
Copy link

coveralls commented May 22, 2019

Coverage Status

Coverage increased (+0.02%) to 87.881% when pulling ba54ebc on Fival85:COLLECTIONS-699 into 252170f on apache:master.

unit tests for equals and toString of the dto entry
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.

Hi @Fival85,

Thank you for your patch. Please address the comments below/above.

Gary

if (!hasNext()) {
throw new NoSuchElementException();
}
final F firstValue = null != firstIterator && firstIterator.hasNext() ? firstIterator.next() : null;
Copy link
Member

Choose a reason for hiding this comment

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

It does not seem necessary to call each delegate iterator's hasNext() method since this method just invoked its own hasNext().

In general, this begs the question of what thread-safety guarantees this class makes. If none, the Javadocs should say so.

* If one iterator has more elements then the other, the result {@link Entry}
* will contain a null value and the value of the not empty child iterator until
* both of the iterator exhausted.
*
Copy link
Member

Choose a reason for hiding this comment

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

You are missing a close tag for 'p' here.

* both of the iterator exhausted.
*
* @param <F> type of first value. The first value of {@link Entry}
*
Copy link
Member

Choose a reason for hiding this comment

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

This blank line is not needed.

* @param <F> the type of the first element
* @param <S> the type of the second element
*/
public static class Entry<F, S> {
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed if you reuse one of:

  • org.apache.commons.collections4.keyvalue.DefaultKeyValue<K, V>
  • org.apache.commons.collections4.keyvalue.DefaultMapEntry<K, V>
  • org.apache.commons.collections4.keyvalue.MultiKey<K>


/**
* Unit test suite for {@link PairingIterator}.
*
Copy link
Member

Choose a reason for hiding this comment

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

Extra line not needed ;-)

@Fival85
Copy link
Author

Fival85 commented Jul 26, 2019

Thank you. I will check it, when i have time for it. at the moment im on vacation :)

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