2

I'm using com.google.common.collect.ComparatorChain to implement a custom comparator for different objects.

Now, the following tests fails, but why?

@Test
public void chainTest() {
    ComparisonChain comparator = ComparisonChain.start()
            .compare("a", "a");
    assertTrue(comparator.result() == 0);
    comparator.compare("a", "b");
    assertTrue(comparator.result() != 0); //fails with AssertionError
}

Afaik the 2nd result should actually be != 0 as a != b?

I could of course reassign the comparator like: comparator = comparator.compare("a", "b");

But that would discard any results that were optained before the reassignment.

membersound
  • 81,582
  • 193
  • 585
  • 1,120

2 Answers2

5

You're ignoring the return value of comparator.compare("a", "b"). Calling compare() doesn't change the state of the existing comparison - it returns a comparison with the right state (which may or may not be the same - as long as the result of the comparison is 0, the state will be "active").

Change your code to use:

comparator = comparator.compare("a", "b");

and it should work.

The idea is that you just keep passing comparisons to the chain, and it will ignore them as soon as it knows the overall result (i.e. when one of them is non-zero). For example:

import com.google.common.collect.ComparisonChain;

public class Test {   
    public static void main(String[] args) {
        int result = ComparisonChain.start()
            .compare("a", "b") // Chain terminates here
            .compare("a", "a") // Irrelevant
            .result();
        System.out.println(result); // < 0 (e.g. -1)
    }
}
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • yes, it's immutable object, exactly like operation methods of BigDecimal classe. – cigno5.5 Feb 12 '15 at 10:53
  • Yes that would work, but discard any results optained before the reassignment. Eg if I first compare `a,b` and then `a,a`, the result would be `= 0` even though it should fail as of the first comparison. So probably I have to check the result explicit each time before reassigning the comparator? – membersound Feb 12 '15 at 10:54
  • @membersound: No, it wouldn't. If you only actually care about the final result, you can just call `compare(...).compare(...).compare(...).result()`. That's how it's designed to be used. The result of `start().compare("a", "b").compare("a", "a").result()` will be less than 0. – Jon Skeet Feb 12 '15 at 10:57
  • No, I meant: `ComparisonChain comparator = ComparisonChain.start().compare("a", b"); comparator = comparator.compare("a", "a"); ` will be `== 0`. – membersound Feb 12 '15 at 11:03
  • @membersound: No, it won't, because the first non-zero comparison terminates the chain - subsequent comparisons are irrelevant. How is your code different from the code in my comment, other than the introduction of a separate local variable (and failing to call `result`)? – Jon Skeet Feb 12 '15 at 11:04
  • @membersound: I've added a short but complete example to my answer to try to persuade you. – Jon Skeet Feb 12 '15 at 11:07
  • @JonSkeet I know that actually *chaining* the compare statements will work. But I have a requirement where I cannot chain, but still want to use the same comparator. Just as written in my OP question. I updated the example to show what I mean. – membersound Feb 12 '15 at 11:11
  • @membersound: Editing your question to basically say that you know you could use the answers is rude - it makes it look like the answerers didn't bother reading your question. It's still not clear *why* you'd expect it to work, or how you want to be able to use the comparison chain - it sounds like it's really just not designed for your use case. Additionally, your question is now incorrect in that it states that a test fails which would actually pass. – Jon Skeet Feb 12 '15 at 11:29
  • @JonSkeet ok sorry, I reverted the edit. Probably sou are right that the class is not designed to work as I'd expected, and I have to recreate the comparisonchain every time I want to add validations that cannot be directly chained. – membersound Feb 12 '15 at 11:39
  • 3
    @membersound: I suggest you ask a new question (rather than disturb this one) to express what you're actually trying to achieve - because it's still *very* unclear to me. The whole point of a comparison chain is that it *is* chained - that the result depends on what's come before it. – Jon Skeet Feb 12 '15 at 11:42
1

Edit:

If you really want it, you may create a "branch":

    ComparisonChain comparator = ComparisonChain.start()
            .compare("a", "a");
    ComparisonChain branch = comparator.compare("a", "b");
    assertTrue(comparator.result() == 0);
    assertTrue(branch.result() != 0);

i.e

-->compare("a", "a")--->result
                    \
                     -->compare("a", "b")--> result
lifus
  • 8,074
  • 1
  • 19
  • 24
  • Yes I hoped I could use the class without creating a so called branch. I thought the `ComparisonChain` would collect the results of each `.compare()` whenever it is added, but that is obviously not the case... – membersound Feb 12 '15 at 11:41