hextriclosan commented on PR #721:
URL:
https://github.com/apache/commons-collections/pull/721#issuecomment-5317927367
Hello @garydgregory,
Thank you for the review and the advice.
What changed:
- Javadoc polished
- Missing tests added
- `next()` now returns a defensive copy, covered by new tests
Two points where I would value your opinion:
**1. `next()` returning a copy.**
The copy is taken before the iterator advances, so the caller's list is
detached regardless of what the successor computation does:
- pros: callers may modify the returned list freely, and the guarantee no
longer depends on internal ordering.
- cons: one extra list allocation per permutation, and since
`PermutationIterator` hands out its internal list, the two classes now differ
internally.
**2. Are equals/hashCode worth keeping?**
They looked reasonable to me, but I have doubts:
- No other iterator in o.a.c.c.iterators overrides them, and neither do the
JDK iterators. Identity semantics seem to be the convention.
- The hash code changes as the iterator advances, so an instance used as a
HashMap key becomes unfindable after the first next().
- Two exhausted iterators built from different collections compare equal,
since equality is defined on the comparator and the next permutation (see
`testEqualsAndHashCodeForExhaustedIterators`). Defensible, as both will emit
nothing further, but surprising.
Thanks again for taking the time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
