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 Comparator
s 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.