CODEWITHSUNDEEP
javacomparatorcomparablecompareto
ryvantage
ryvantage

Reputation: 13496

Comparison method violates its general contract -- But I might want to?

I am trying to write a table sorter that will -always- sort null values to the bottom. So I have written a "wrapper" class that implements Comparable:

public class WrappedBigDecimal implements Comparable<WrappedBigDecimal> {
    public final BigDecimal value;

    public WrappedBigDecimal(BigDecimal value) {
        this.value = value;
    }

    @Override
    public int compareTo(WrappedBigDecimal o) {
        if (value == null && (o == null || o.value == null)) { // both are null, thus equal
            return 0;
        } else if (value == null && (o != null && o.value != null)) { // value is null and compared value isn't
            return -1;
        } else if (value != null && (o == null || o.value == null)) {
            return 1;
        } else {
            return value.compareTo(o.value);
        }
    }

    @Override
    public String toString() {
        return String.valueOf(value);
    }

}

You'll notice that the compareTo method only does null checks and then defers to the compareTo method of the wrapped value's class.

Then I wrote a RowSorter whose Comparator checks for the SortOrder

public class WrappedNumberSorter extends TableRowSorter<TableModel> {

    public WrappedNumberSorter(TableModel model) {
        super(model);
    }

    @Override
    public Comparator<?> getComparator(final int column) {
        Comparator c = new Comparator() {
            @Override
            public int compare(Object o1, Object o2) {
                boolean ascending = getSortKeys().get(0).getSortOrder() == SortOrder.ASCENDING;

                if (o1 instanceof WrappedBigDecimal && ((WrappedBigDecimal)o1).value == null) {
                    if(ascending)
                        return 1;
                    else
                        return -1;
                } else if (o2 instanceof WrappedBigDecimal && ((WrappedBigDecimal)o2).value == null) {
                    if(ascending)
                        return -1;
                    else
                        return 1;
                } else {
                    return ((Comparable<Object>) o1).compareTo(o2);
                }

            }
        };
        return c;
    }
}

But this throws an error (can't figure out why, can't reproduce -- read on):

java.lang.IllegalArgumentException: Comparison method violates its general contract!

Luckily, the error doesn't seem to affect anything because everything is working as intended.

I saw this question (java.lang.IllegalArgumentException: Comparison method violates its general contract) and I THINK my problem is that my comparison is not transitive. Although I'm not sure because if A == B and B == C then I think mine would also return A == C but I'm getting spun around trying to think about it.

Anyways, my questions are:

  1. Is there a potential danger to intentionally making the row sorter sort nulls to the bottom in this way?
  2. This SEEMS transitive to me. Am I, indeed, violating the transitive contract of compareTo here? Or is there another contract that it must adhere to and I'm unaware and that's what throws the error?

I wrote a method to test my row sorter and I am unable to reproduce the error:

public static void main(String[] args) {
    JFrame frame = new JFrame();
    JTable table = new JTable();
    JScrollPane jsp = new JScrollPane(table);

    String[] headers = new String[] {"h1", "h2"};

    Object[][] data = new Object[10][2];
    for(int i = 0; i < 7; i++) {
        data[i][0] = new WrappedBigDecimal(BigDecimal.TEN.multiply(BigDecimal.valueOf(i % 3 == 0 ? (i*i*-1) : (i*i))));
    }

    data[7][0] = new WrappedBigDecimal(null);
    data[8][0] = new WrappedBigDecimal(null);
    data[9][0] = new WrappedBigDecimal(null);

    for(int i = 10; i < 17; i++) {
        data[i-10][1] = new WrappedBigDecimal(BigDecimal.TEN.multiply(BigDecimal.valueOf(i % 3 == 0 ? (i*i*-1) : (i*i))));
    }

    data[7][1] = new WrappedBigDecimal(null);
    data[8][1] = new WrappedBigDecimal(null);
    data[9][1] = new WrappedBigDecimal(null);

    table.setModel(new DefaultTableModel(data, headers) {
        @Override
        public boolean isCellEditable(int row, int column) {
            return false;
        }

        @Override
        public Class<?> getColumnClass(int columnIndex) {
            return BigDecimal.class;
        }
    });

    table.setRowSorter(new WrappedNumberSorter(table.getModel()));

    frame.add(jsp);
    frame.setSize(200,400);
    frame.setLocationRelativeTo(null);
    frame.setVisible(true);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

Everything seems to be working smoothly.

What am I missing?

enter image description here

EDIT (7/11/18): Attempting to get rid of the WrappedBigDecimal solution and implement Matt McHenry's solution presents another problem. I simplified the RowSorter to this:

static class NullsLastSorter extends TableRowSorter<TableModel> {

    public NullsLastSorter(TableModel model) {
        super(model);
    }

    @Override
    public Comparator<?> getComparator(int column) {
        return Comparator.<Optional<BigDecimal>, Boolean>comparing(Optional::isPresent).reversed().thenComparing(o -> o.orElse(BigDecimal.ONE));
    }
}

And tested it with:

public static void main(String[] args) {
    JFrame frame = new JFrame();
    JTable table = new JTable();
    JScrollPane jsp = new JScrollPane(table);

    String[] headers = new String[] {"h1", "h2"};

    Object[][] data = new Object[10][2];
    for(int i = 0; i < 7; i++) {
        data[i][0] = BigDecimal.TEN.multiply(BigDecimal.valueOf(i % 3 == 0 ? (i*i*-1) : (i*i)));
    }

    data[7][0] = null;
    data[8][0] = null;
    data[9][0] = null;

    for(int i = 10; i < 17; i++) {
        data[i-10][1] = BigDecimal.TEN.multiply(BigDecimal.valueOf(i % 3 == 0 ? (i*i*-1) : (i*i)));
    }

    data[7][1] = null;
    data[8][1] = null;
    data[9][1] = null;



    table.setModel(new DefaultTableModel(data, headers) {
        @Override
        public boolean isCellEditable(int row, int column) {
            return false;
        }

        @Override
        public Class<?> getColumnClass(int columnIndex) {
            return BigDecimal.class;
        }
    });

    table.setRowSorter(new NullsLastSorter(table.getModel()));

    frame.add(jsp);
    frame.setSize(200,400);
    frame.setLocationRelativeTo(null);
    frame.setVisible(true);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

And now when I try to sort I get an error:

Exception in thread "AWT-EventQueue-0" java.lang.ClassCastException: java.math.BigDecimal cannot be cast to java.util.Optional
at java.util.Comparator.lambda$comparing$77a9974f$1(Comparator.java:469)
at java.util.Collections$ReverseComparator2.compare(Collections.java:5178)
at java.util.Comparator.lambda$thenComparing$36697e65$1(Comparator.java:216)
at javax.swing.DefaultRowSorter.compare(DefaultRowSorter.java:968)
at javax.swing.DefaultRowSorter.access$100(DefaultRowSorter.java:112)
at javax.swing.DefaultRowSorter$Row.compareTo(DefaultRowSorter.java:1376)
at javax.swing.DefaultRowSorter$Row.compareTo(DefaultRowSorter.java:1366)
at java.util.ComparableTimSort.countRunAndMakeAscending(ComparableTimSort.java:320)
at java.util.ComparableTimSort.sort(ComparableTimSort.java:188)
at java.util.Arrays.sort(Arrays.java:1246)
at javax.swing.DefaultRowSorter.sort(DefaultRowSorter.java:607)
at javax.swing.DefaultRowSorter.setSortKeys(DefaultRowSorter.java:319)
at javax.swing.DefaultRowSorter.toggleSortOrder(DefaultRowSorter.java:480)
at javax.swing.plaf.basic.BasicTableHeaderUI$MouseInputHandler.mouseClicked(BasicTableHeaderUI.java:112)
at java.awt.AWTEventMulticaster.mouseClicked(AWTEventMulticaster.java:270)
at java.awt.Component.processMouseEvent(Component.java:6536)
at javax.swing.JComponent.processMouseEvent(JComponent.java:3324)
at java.awt.Component.processEvent(Component.java:6298)
at java.awt.Container.processEvent(Container.java:2236)
at java.awt.Component.dispatchEventImpl(Component.java:4889)
at java.awt.Container.dispatchEventImpl(Container.java:2294)
at java.awt.Component.dispatchEvent(Component.java:4711)
at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4888)
at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4534)
at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4466)
at java.awt.Container.dispatchEventImpl(Container.java:2280)
at java.awt.Window.dispatchEventImpl(Window.java:2746)
at java.awt.Component.dispatchEvent(Component.java:4711)
at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:758)
at java.awt.EventQueue.access$500(EventQueue.java:97)
at java.awt.EventQueue$3.run(EventQueue.java:709)
at java.awt.EventQueue$3.run(EventQueue.java:703)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
at java.awt.EventQueue$4.run(EventQueue.java:731)
at java.awt.EventQueue$4.run(EventQueue.java:729)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:76)
at java.awt.EventQueue.dispatchEvent(EventQueue.java:728)
at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201)
at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

Upvotes: 0

Views: 1140

Answers (2)

Matt McHenry
Matt McHenry

Reputation: 20929

Contract Violations

I see two pieces of the contract being violated:

  • WrappedBigDecimal.compareTo(null)

The javadoc for Comparable says that compareTo(null) must always throw a NullPointerException. Your method does not obey this.

  • WrappedNumberSorter.getComparator()'s handling of WrappedBigDecimal(null)

Consider this code:

WrappedBigDecimal x = new WrappedBigDecimal(null);
WrappedBigDecimal y = new WrappedBigDecimal(null);

... along with this quote from the javadoc for Comparator:

The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y.

Using your code (assuming ascending == true), we have compare(x, y) == 1. But compare(y, x) == 1. Since sgn(1) != -sgn(1), this is a violation of the contract of the Comparator interface.

General Comments

Your overall goal of sorting nulls to the beginning or end is fine, in fact it's very common. But you're missing a lot of opportunities to use library code.

Comparator helper methods

In Java, it's almost always easier to use the static helper methods in the Comparator class than to write your own compare() methods by hand. Indeed, there is a tailor-made one just for your use case: nullsLast().

Optional

Instead of writing your own wrapper class to deal with null values for your BigDecimal, just use Optional<BigDecimal>. This is exactly the sort of thing it's meant for.

Complete Solution

Since Optional<T> is a very general container class, it can't implement Comparable<Optional<T>> itself (there's no guarantee the type it's wrapping is Comparable). But Comparator's static helper methods make it easy to do ourselves.

First we sort by whether there is a value present or not, making sure that empty optionals go to the end rather than the beginning.

Then, when we have a pair of optionals whose "presence" is equivalent (either both present or both absent), we give a simple lambda to extract the value to compare. orElse() is just what we need: if a value is present, get it and compare it; if a value is not present, provide a fallback. (Remember, that last case only happens when both Optionals being compared are empty, so it doesn't matter what value we use as the fallback, it's always just compared to itself, giving the result we want: two empty optionals are equivalent.)

Comparator.<Optional<BigDecimal>,Boolean>comparing(Optional::isPresent)
          .reversed() //default ordering of booleans is false before true
          .thenComparing(o -> o.orElse(BigDecimal.ONE))

Upvotes: 2

Joe
Joe

Reputation: 1342

You can, get rid of the wrapper class. And in the sorting method, check the values of each object and sort them accordingly. For example, you want all null instances to be put to the bottom. If one, or both, are null, treat them as values of -1

i.e)

@Override
public Comparator<?> getComparator(final int column) {
    return new Comparator() {
        @Override
        public int compare(Object o1, Object o2) {
            if (o1 == null || o2 == null)
                return -1;
            /**
             * Do the rest of your checks here
             */
            return o1.compareTo(o2);
        }
    };
}

Also, it looks like you are expecting "non-BigDecimal" objects to be compared in your BigDecimal comparable object.

A) If BigDecimal already implements Comparable, there is no need to re-implement the interface. Just override the existing method from the superclass.

B) WrappedBigDecimal is the Generic type, your code seems to be accepting other types that are not of this type, which may not be important here, but can definitely be causing you errors

Upvotes: 0

Related Questions