IterableAssert.doesNotHaveDuplicates performance enhancement

[assertj]

IterableAssert.doesNotHaveDuplicates performance enhancement

[twillouer]

I'm not ok with your correction, you forgot to set the Comparator in the StandardComparaisonStrategy.newSetUsingComparisonStrategy

try with this test (in Iterables_assertDoesNotHaveDuplicates_Test for example) :

    @Test
    public void should_fail_if_actual_contains_duplicates_array() {
        AssertionInfo info = someInfo();
        Collection<String[]> actual = newArrayList(new String[]{"Luke", "Yoda"}, new String[]{"Luke", "Yoda"});
        Collection<String[]> duplicate = new LinkedHashSet<String[]>();
        duplicate.add(new String[]{"Luke", "Yoda"});
        try {
            iterables.assertDoesNotHaveDuplicates(info, actual);
        } catch (AssertionError e) {
            return;
        }
        failBecauseExpectedAssertionErrorWasNotThrown();
    }

The test is fine in the "old" master branch, but not in your correction.
In fact, the Object.areEqual have special test for array, so if you don't call him, this will fail.

[joel-costigliola]

You're right, good point.
I can either go back to your initial solution or define a comparator in StandardComparaisonStrategy using areEqual to test for equality but I'm not so sure of the comparator approach since, beside equality, it does not mean anything to compare objects.
WDYT ?

[twillouer]

I'm trying a naïve approch like

  protected Set<Object> newSetUsingComparisonStrategy() {
    return new TreeSet<Object>(new Comparator<Object>() {
        @Override
        public int compare(Object o1, Object o2) {
            return Objects.areEqual(o1, o2) ? 0 : -1;
        }
    });
  }

works fine for Array, but new problem for handle null :

public class StandardComparisonStrategy_duplicatesFrom_Test extends AbstractTest_StandardComparisonStrategy {

  @Test
  public void should_return_existing_duplicates() {
    Iterable<?> duplicates = standardComparisonStrategy
        .duplicatesFrom(newArrayList("Merry", "Frodo", null, null, "Merry", "Sam", "Frodo"));
    assertEquals(3, sizeOf(duplicates));
    assertTrue(standardComparisonStrategy.iterableContains(duplicates, "Frodo"));
    assertTrue(standardComparisonStrategy.iterableContains(duplicates, "Merry"));
    assertTrue(standardComparisonStrategy.iterableContains(duplicates, null));
  }

this test doesn't work anymore...
Will try harder and see if I can find a better approch (when my daughter sleep :P)

[joel-costigliola]

I was implementing the comparator solution to see what it looks like, handling null like this makes your test succeeds :

  private StandardComparisonStrategy() {
    // define a comparator so that we can use areEqual to compare objects in Set collections
    // the "less than" comparison does not make much sense here but need to be defined. 
    comparator = new Comparator<Object>() {
      @Override
      public int compare(Object o1, Object o2) {
        if (areEqual(o1, o2)) return 0;
        // here, both objects can't be null but one can
        if (o1 == null) return -1;
        if (o2 == null) return 1;
        return o1.hashCode() < o2.hashCode() ? -1 : 1;
      }
    };
  }

[twillouer]

for now, the best than I have :

  protected Set<Object> newSetUsingComparisonStrategy() {
    return new TreeSet<Object>(new Comparator<Object>() {
      @Override
      public int compare(Object o1, Object o2) {
        if (Objects.areEqual(o1, o2)) {
          return 0;
        }
        return Objects.hashCodeFor(o1) - Objects.hashCodeFor(o2) < 0 ? -1 : 1;
      }
    });
  }

Work with :

  @Test
  public void should_return_existing_duplicates_array() {
    Iterable<?> duplicates = standardComparisonStrategy.duplicatesFrom(newArrayList(new String[] { "Merry" },
        new String[] { "Frodo" }, new String[] { null }, new String[] { null }, new String[] { "Merry" },
        new String[] { "Sam" }, new String[] { "Frodo" }));
    assertTrue("must contains Frodo", standardComparisonStrategy.iterableContains(duplicates, new String[] { "Frodo" }));
    assertTrue("must contains Merry", standardComparisonStrategy.iterableContains(duplicates, new String[] { "Merry" }));
    assertTrue("must contains null", standardComparisonStrategy.iterableContains(duplicates, new String[] { null }));
    assertEquals(3, sizeOf(duplicates));
  }

in StandardComparisonStrategy_duplicatesFrom_Test

I try this last test with your strategy and he doen't pass

[twillouer]

forget this, I have random failed..not sure hashCode is a good things..

[twillouer]

can I change Objects.hashCodeFor to handle Array ?

[joel-costigliola]

Yeah was having the same problem.
William, let's consider chatting with gmail if you like , github not being very handy for that ;-)
My gmail is : joel.costigliola(at)gmail.com

[joel-costigliola]

Yep, we should do that, it will make it consistent with areEqual