Skip to content
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

Use Comparable instead of dedicated implementations #25478

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
* {@code true} or {@code false} first.
*
* @author Keith Donald
* @author Eugene Rabii
* @since 1.2.2
*/
@SuppressWarnings("serial")
Expand Down Expand Up @@ -63,8 +64,9 @@ public BooleanComparator(boolean trueLow) {


@Override
public int compare(Boolean v1, Boolean v2) {
return (v1 ^ v2) ? ((v1 ^ this.trueLow) ? 1 : -1) : 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some time ago I had an answer about this here : https://stackoverflow.com/questions/55598277/is-there-a-useful-difference-between-p-q-and-p-q-for-booleans/55601399#55601399 and it turns out that the performance of XOR vs plain != is same. This also makes it far easier to read

public int compare(Boolean left, Boolean right) {
int multiplier = this.trueLow ? -1 : 1;
return multiplier * Boolean.compare(left, right);
}


Expand All @@ -76,7 +78,7 @@ public boolean equals(@Nullable Object other) {

@Override
public int hashCode() {
return getClass().hashCode() * (this.trueLow ? -1 : 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since java-8 there is a way to get the hashCode without calling into getClass

return Boolean.hashCode(this.trueLow);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,18 @@
* Mainly for internal use in other Comparators, when supposed
* to work on Comparables.
*
* @deprecated use jdk-8 Comparator::naturalOrder
* @author Keith Donald
* @since 1.2.2
* @param <T> the type of comparable objects that may be compared by this comparator
* @see Comparable
*/
@Deprecated
public class ComparableComparator<T extends Comparable<T>> implements Comparator<T> {

/**
* A shared instance of this default comparator.
*
* @see Comparators#comparable()
*/
@SuppressWarnings("rawtypes")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,49 +29,44 @@ public abstract class Comparators {

/**
* Return a {@link Comparable} adapter.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used Comparator::naturalOrder and Comparator::nullLast/nullFirst instead of NullSafeComparator

* @see ComparableComparator#INSTANCE
*/
@SuppressWarnings("unchecked")
public static <T> Comparator<T> comparable() {
return ComparableComparator.INSTANCE;
return (Comparator<T>) Comparator.naturalOrder();
}

/**
* Return a {@link Comparable} adapter which accepts
* null values and sorts them lower than non-null values.
* @see NullSafeComparator#NULLS_LOW
*/
@SuppressWarnings("unchecked")
public static <T> Comparator<T> nullsLow() {
return NullSafeComparator.NULLS_LOW;
return (Comparator<T>) Comparator.nullsLast(Comparator.naturalOrder());
}

/**
* Return a decorator for the given comparator which accepts
* null values and sorts them lower than non-null values.
* @see NullSafeComparator#NullSafeComparator(boolean)
*/
public static <T> Comparator<T> nullsLow(Comparator<T> comparator) {
return new NullSafeComparator<>(comparator, true);
return Comparator.nullsLast(comparator);
}

/**
* Return a {@link Comparable} adapter which accepts
* null values and sorts them higher than non-null values.
* @see NullSafeComparator#NULLS_HIGH
*/
@SuppressWarnings("unchecked")
public static <T> Comparator<T> nullsHigh() {
return NullSafeComparator.NULLS_HIGH;
return (Comparator<T>) Comparator.nullsFirst(Comparator.naturalOrder());
}

/**
* Return a decorator for the given comparator which accepts
* null values and sorts them higher than non-null values.
* @see NullSafeComparator#NullSafeComparator(boolean)
*/
public static <T> Comparator<T> nullsHigh(Comparator<T> comparator) {
return new NullSafeComparator<>(comparator, false);
return Comparator.nullsFirst(comparator);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ public class NullSafeComparator<T> implements Comparator<T> {
@SuppressWarnings("rawtypes")
public static final NullSafeComparator NULLS_HIGH = new NullSafeComparator<>(false);


private final Comparator<T> nonNullComparator;

private final boolean nullsLow;
Expand All @@ -71,7 +70,7 @@ public class NullSafeComparator<T> implements Comparator<T> {
*/
@SuppressWarnings("unchecked")
private NullSafeComparator(boolean nullsLow) {
this.nonNullComparator = ComparableComparator.INSTANCE;
this.nonNullComparator = (Comparator<T>) Comparator.naturalOrder();
this.nullsLow = nullsLow;
}

Expand All @@ -92,17 +91,9 @@ public NullSafeComparator(Comparator<T> comparator, boolean nullsLow) {


@Override
public int compare(@Nullable T o1, @Nullable T o2) {
if (o1 == o2) {
return 0;
}
if (o1 == null) {
return (this.nullsLow ? -1 : 1);
}
if (o2 == null) {
return (this.nullsLow ? 1 : -1);
}
return this.nonNullComparator.compare(o1, o2);
public int compare(@Nullable T left, @Nullable T right) {
Comparator<T> comparator = this.nullsLow ? Comparator.nullsFirst(this.nonNullComparator) : Comparator.nullsLast(this.nonNullComparator);
return comparator.compare(left, right);
}


Expand All @@ -121,7 +112,7 @@ public boolean equals(@Nullable Object other) {

@Override
public int hashCode() {
return this.nonNullComparator.hashCode() * (this.nullsLow ? -1 : 1);
return Boolean.hashCode(this.nullsLow);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,34 +29,35 @@
* @author Keith Donald
* @author Chris Beams
* @author Phillip Webb
* @author Eugene Rabii
*/
class BooleanComparatorTests {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests have changed because the documentation of Boolean::compare does not guarantee -1, 0, 1; but it does less than 1, 0, greater than 1

@Test
void shouldCompareWithTrueLow() {
Comparator<Boolean> c = new BooleanComparator(true);
assertThat(c.compare(true, false)).isEqualTo(-1);
assertThat(c.compare(true, false)).isLessThan(0);
assertThat(c.compare(Boolean.TRUE, Boolean.TRUE)).isEqualTo(0);
}

@Test
void shouldCompareWithTrueHigh() {
Comparator<Boolean> c = new BooleanComparator(false);
assertThat(c.compare(true, false)).isEqualTo(1);
assertThat(c.compare(true, false)).isGreaterThan(0);
assertThat(c.compare(Boolean.TRUE, Boolean.TRUE)).isEqualTo(0);
}

@Test
void shouldCompareFromTrueLow() {
Comparator<Boolean> c = BooleanComparator.TRUE_LOW;
assertThat(c.compare(true, false)).isEqualTo(-1);
assertThat(c.compare(true, false)).isLessThan(0);
assertThat(c.compare(Boolean.TRUE, Boolean.TRUE)).isEqualTo(0);
}

@Test
void shouldCompareFromTrueHigh() {
Comparator<Boolean> c = BooleanComparator.TRUE_HIGH;
assertThat(c.compare(true, false)).isEqualTo(1);
assertThat(c.compare(true, false)).isGreaterThan(0);
assertThat(c.compare(Boolean.TRUE, Boolean.TRUE)).isEqualTo(0);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void comparableComparator() {
Comparator<String> c = new ComparableComparator<>();
String s1 = "abc";
String s2 = "cde";
assertThat(c.compare(s1, s2) < 0).isTrue();
assertThat(c.compare(s1, s2)).isLessThan(0);
}

@SuppressWarnings({ "unchecked", "rawtypes" })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ class NullSafeComparatorTests {
@Test
void shouldCompareWithNullsLow() {
Comparator<String> c = NullSafeComparator.NULLS_LOW;
assertThat(c.compare(null, "boo") < 0).isTrue();
assertThat(c.compare(null, "boo")).isLessThan(0);
}

@SuppressWarnings("unchecked")
@Test
void shouldCompareWithNullsHigh() {
Comparator<String> c = NullSafeComparator.NULLS_HIGH;
assertThat(c.compare(null, "boo") > 0).isTrue();
assertThat(c.compare(null, null) == 0).isTrue();
assertThat(c.compare(null, "boo")).isGreaterThan(0);
assertThat(c.compare(null, null)).isZero();
}

}