-2

I want to sort a list of field names alphabetically however I need to include a condition in the doCompare method of the comparator so that if the field name is "pk" that will always be sorted to the top of the list. What I have is below but I'm not sure if I'm taking the right approach, particualrly with the reurn value of -1000. Any advice on this would be much appreciated.

    @Override
    public int doCompare(Object firstRec, Object secondRec)
    {
        MyField firstField = (MyField) firstRec;
        MyField secondField = (MyField ) secondRec;
        if(firstField.name() == "pk")
        {
            return -1000;
        }

        return StringUtils.compareStrings(firstField.name().toLowerCase(), secondField.name().toLowerCase());
     }
Jonny
  • 59
  • 1
  • 7
  • 1
    Before you ask us, you should write some tests and see your code in action. You don't need us to check if your code works or not. – vanje Aug 19 '22 at 15:18
  • Just return any negative value. `-1` is ok. But you also need to consider the case when `secondRec` is `"pk"` – peterz Aug 19 '22 at 15:18
  • 2
    Don't compare strings with `==`, use `.equals`. – Andy Turner Aug 19 '22 at 15:32
  • 1
    The thing which is wrong here is that you also need to return a positive value if `secondField.name()` is `pk` as well; and return zero if _both_ equal `pk`. – Andy Turner Aug 19 '22 at 15:33
  • 1
    See: [How do I compare strings in Java?](https://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java) – Jesper Aug 19 '22 at 15:41

2 Answers2

1

The requirements of a Comparator (and, by extension, methods which are supposed to act like Comparator.compare) are described in the Javadoc:

The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y. (This implies that compare(x, y) must throw an exception if and only if compare(y, x) throws an exception.)

The implementor must also ensure that the relation is transitive: ((compare(x, y)>0) && (compare(y, z)>0)) implies compare(x, z)>0.

Finally, the implementor must ensure that compare(x, y)==0 implies that sgn(compare(x, z))==sgn(compare(y, z)) for all z.

Assuming StringUtils.compareStrings correctly implements these requirements, the thing you've got wrong is the first requirement: you also need to consider the cases when secondField is also pk:

The general pattern for writing correct Comparators is:

int firstComparison = /* compare something about firstField and secondField */;
if (firstComparison != 0) {
  return firstComparison;
}

int secondComparison = /* compare something else about firstField and secondField */;
if (secondComparison != 0) {
  return secondComparison;
}

// ...

return 0;

Applying that here:

int pkComparison = Boolean.compare(secondField.name().equals("pk"), firstField.name().equals("pk"));
if (pkComparison != 0) {
  return pkComparison;
}

int compareStringsComparison = StringUtils.compareStrings(firstField.name().toLowerCase(), secondField.name().toLowerCase());
if (compareStringsComparison != 0) {
  return compareStringsComparison;
}

return 0;

Obviously, the last if statement is redundant, because you always return compareStringsComparison whether or not it is zero; so you could write simply:

return StringUtils.compareStrings(firstField.name().toLowerCase(), secondField.name().toLowerCase());

I would recommend sticking to the compare/check and return/finally return 0 pattern, because it's easier to slot in additional conditions later. But it's not terrible either way.

Andy Turner
  • 137,514
  • 11
  • 162
  • 243
1

The new static methods of class Comparator available since Java 8 are very handy to create a multi-criteria Comparator like in your case.

You could try something like this:

List<String> list = ... ;
list.sort(
  Comparator.comparingBoolean("PK"::equals)
  .thenComparing(StringUtils::compare)
);

You may need to use .reversed() in case the order is the opposite of what you want.

The great advantage of Comparator.comparing / Comparator.comparingXXX is that you don't need to twist your mind to get the correct behavior when to return a positive, negative or 0 value.

The Comparator.thenComparing dos proper chaining, i.e. it checks further criterias only when needed, only when previous comparisons returned 0.

If your list may contain null values, there are also methods to handle them properly. This isn't the case in this short example.

QuentinC
  • 12,311
  • 4
  • 24
  • 37