-
-
Notifications
You must be signed in to change notification settings - Fork 704
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
Preserves the original order of elements when returning the duplicates #3333
Conversation
// for elements of type Set collections, ensure each element is added only once | ||
Set<Object> alreadyAddedDuplicates = newSetUsingComparisonStrategy(); | ||
for (Object element : iterable) { | ||
if (duplicates.contains(element)&& !alreadyAddedDuplicates.contains(element)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can get rid of addedAddDuplicates entirely, if you make the second part of the if test !duplicatesInOriginalOrder.contains(element)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For collection of array type e.g. newArrayList(array("Merry"), array("Frodo"), array("Merry")) TreeSet uses custom comparator to identify the duplicates. This is done by checking the deeply equals for two array( areEquals method). so identical arrays like array("Merry") are seen as duplicates by TreeSet.
since we don't have this comparator in LinkedHashSet, it will not be able to identify two arrays with same elements as duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing a lot of context here, so ignore me if that is the case. Instead of introducing a private method, can this be done in the duplicatesFrom
method instead, like replacing Lines 34 with a LinkedHashSet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
earlier it was not possible but for option-2 (first duplicate first in response) implementation its possible.
I have made that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks okay to me. I just don't understand why so many tests have been adapted. I would expect that those can remain as before, can't they?
Tests are changed because order of duplicate elements in response is changed now. e.g if the list has (6, -8, 6, 8) previous implementation with TreeSet + AbsoluteValueComparator would return (6, 8) |
@ranjitshinde91 there are tests failing, can you look at them ? |
@joel-costigliola there were few formatting errors, fixed those. |
… collection i.e first duplicated element first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
Looks good to me |
Integrated thanks @ranjitshinde91, @Bananeweizen and @kulgan ! |
Check List:
Following the contributing guidelines will make it easier for us to review and accept your PR.