"Vergleichsmethode verstößt gegen allgemeinen Vertrag!"

187

Kann mir jemand in einfachen Worten erklären, warum dieser Code eine Ausnahme auslöst: "Die Vergleichsmethode verstößt gegen ihren allgemeinen Vertrag!" Und wie behebe ich sie?

private int compareParents(Foo s1, Foo s2) {
    if (s1.getParent() == s2) return -1;
    if (s2.getParent() == s1) return 1;
    return 0;
}
n00bster
quelle
1
Wie lautet der Name und die Klasse der Ausnahme? Ist es eine IllegalArgumentException? Wenn ich raten müsste, würde ich denken, dass Sie s1.getParent().equals(s2)stattdessen tun sollten s1.getParent() == s2.
Freiheit
Und die Ausnahme, die auch geworfen wird.
Matthew Farwell
2
Ich weiß nicht viel über Java oder über Java-Vergleichs-APIs, aber diese Vergleichsmethode scheint absolut falsch zu sein. Angenommen, es s1ist das Elternteil von s2und s2ist nicht das Elternteil von s1. Dann compareParents(s1, s2)ist 0, aber compareParents(s2, s1)ist 1. Das macht keinen Sinn. (Außerdem ist es nicht transitiv, wie unten erwähnt.)
mqp
4
Dieser Fehler scheint nur von einer bestimmten Bibliothek cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/src/share/…
Peter Lawrey
In Java können Sie equals (Rückgabe eines Booleschen Werts) oder compareTo (Rückgabe -1, 0 oder +1) verwenden. Überschreiben Sie diese Funktionen in Ihrer Foo-Klasse und überprüfen Sie danach s1.getParent (). Equals (s2) ...
Mualig

Antworten:

260

Ihr Komparator ist nicht transitiv.

Sei Ader Elternteil von Bund Bsei der Elternteil von C. Seit A > Bund B > Cdann muss es so sein A > C. Wenn Ihr Komparator jedoch auf Aund aufgerufen wird C, gibt er Null zurück, was bedeutetA == C . Dies verstößt gegen den Vertrag und löst daher die Ausnahme aus.

Es ist ziemlich nett von der Bibliothek, dies zu erkennen und Sie wissen zu lassen, anstatt sich unberechenbar zu verhalten.

Eine Möglichkeit, die Transitivitätsanforderung zu erfüllen, compareParents()besteht darin, die getParent()Kette zu durchlaufen , anstatt nur den unmittelbaren Vorfahren zu betrachten.

NPE
quelle
3
Eingeführt in Java 7's java.util.Arrays.sort stackoverflow.com/questions/7849539/…
leonbloy
46
Die Tatsache, dass die Bibliothek dies erkennt, ist fantastisch. Jemand in der Sonne hat es verdient, einen Riesen rauszuwerfen. Gern geschehen .
Qix - MONICA wurde am
Können Sie diese Antwort etwas verallgemeinern, um diese Frage als Referenzbeitrag nützlicher zu machen?
Bernhard Barker
1
@ Qix - so sehr ich Sun liebe, wurde dies in Java 7 unter dem Oracle-Banner
hinzugefügt
1
@isapir Verdammt! Guter Fang.
Qix - MONICA wurde am
38

Nur weil ich das bekommen habe, als ich diesen Fehler gegoogelt habe, war mein Problem, dass ich es hatte

if (value < other.value)
  return -1;
else if (value >= other.value)
  return 1;
else
  return 0;

Das value >= other.valuesollte (offensichtlich) tatsächlich value > other.valueso sein, dass Sie tatsächlich 0 mit gleichen Objekten zurückgeben können.

you786
quelle
7
Ich muss hinzufügen, dass wenn einer von Ihnen valueein NaN ist (wenn valuees ein doubleoder ist float), es auch scheitern würde.
Matthieu
22

Die Vertragsverletzung führt häufig dazu, dass der Komparator beim Vergleich von Objekten nicht den richtigen oder konsistenten Wert liefert. Beispielsweise möchten Sie möglicherweise einen Zeichenfolgenvergleich durchführen und leere Zeichenfolgen zwingen, bis zum Ende zu sortieren mit:

if ( one.length() == 0 ) {
    return 1;                   // empty string sorts last
}
if ( two.length() == 0 ) {
    return -1;                  // empty string sorts last                  
}
return one.compareToIgnoreCase( two );

Dies übersieht jedoch den Fall, in dem sowohl eins als auch zwei leer sind - und in diesem Fall wird der falsche Wert zurückgegeben (1 anstelle von 0, um eine Übereinstimmung anzuzeigen), und der Komparator meldet dies als Verstoß. Es sollte geschrieben worden sein als:

if ( one.length() == 0 ) {
    if ( two.length() == 0 ) {
        return 0;               // BOth empty - so indicate
    }
    return 1;                   // empty string sorts last
}
if ( two.length() == 0 ) {
    return -1;                  // empty string sorts last                  
}
return one.compareToIgnoreCase( two );
CESDewar
quelle
13

Selbst wenn Ihr compareTo theoretisch die Transitivität enthält, bringen manchmal subtile Fehler die Dinge durcheinander ... wie z. B. Gleitkomma-Rechenfehler. Es ist mir passiert. Das war mein Code:

public int compareTo(tfidfContainer compareTfidf) {
    //descending order
    if (this.tfidf > compareTfidf.tfidf)
        return -1;
    else if (this.tfidf < compareTfidf.tfidf)
        return 1;
    else
        return 0;

}   

Die transitive Eigenschaft ist eindeutig gültig, aber aus irgendeinem Grund habe ich die IllegalArgumentException erhalten. Und es stellt sich heraus, dass aufgrund winziger Fehler in der Gleitkomma-Arithmetik die Rundungsfehler dazu führten, dass die transitive Eigenschaft dort brach, wo sie nicht sollte! Also habe ich den Code umgeschrieben, um wirklich winzige Unterschiede 0 zu berücksichtigen, und es hat funktioniert:

public int compareTo(tfidfContainer compareTfidf) {
    //descending order
    if ((this.tfidf - compareTfidf.tfidf) < .000000001)
        return 0;
    if (this.tfidf > compareTfidf.tfidf)
        return -1;
    else if (this.tfidf < compareTfidf.tfidf)
        return 1;
    return 0;
}   
Ali Mizan
quelle
2
Das war hilfreich! Mein Code war logisch in Ordnung, aber aufgrund des Präzisionsproblems ist ein Fehler aufgetreten.
JSong
6

In unserem Fall wurde dieser Fehler angezeigt, weil wir versehentlich die Vergleichsreihenfolge von s1 und s2 umgedreht hatten. Also pass auf. Es war offensichtlich viel komplizierter als das Folgende, aber dies ist eine Illustration:

s1 == s2   
    return 0;
s2 > s1 
    return 1;
s1 < s2 
    return -1;
Onkel Iroh
quelle
3

Java überprüft die Konsistenz nicht im engeren Sinne, sondern benachrichtigt Sie nur, wenn ernsthafte Probleme auftreten. Es gibt Ihnen auch nicht viele Informationen über den Fehler.

Ich war verwirrt über das, was in meinem Sortierer passiert, und habe einen strengen Konsistenzchecker erstellt. Vielleicht hilft Ihnen das:

/**
 * @param dailyReports
 * @param comparator
 */
public static <T> void checkConsitency(final List<T> dailyReports, final Comparator<T> comparator) {
  final Map<T, List<T>> objectMapSmallerOnes = new HashMap<T, List<T>>();

  iterateDistinctPairs(dailyReports.iterator(), new IPairIteratorCallback<T>() {
    /**
     * @param o1
     * @param o2
     */
    @Override
    public void pair(T o1, T o2) {
      final int diff = comparator.compare(o1, o2);
      if (diff < Compare.EQUAL) {
        checkConsistency(objectMapSmallerOnes, o1, o2);
        getListSafely(objectMapSmallerOnes, o2).add(o1);
      } else if (Compare.EQUAL < diff) {
        checkConsistency(objectMapSmallerOnes, o2, o1);
        getListSafely(objectMapSmallerOnes, o1).add(o2);
      } else {
        throw new IllegalStateException("Equals not expected?");
      }
    }
  });
}

/**
 * @param objectMapSmallerOnes
 * @param o1
 * @param o2
 */
static <T> void checkConsistency(final Map<T, List<T>> objectMapSmallerOnes, T o1, T o2) {
  final List<T> smallerThan = objectMapSmallerOnes.get(o1);

  if (smallerThan != null) {
    for (final T o : smallerThan) {
      if (o == o2) {
        throw new IllegalStateException(o2 + "  cannot be smaller than " + o1 + " if it's supposed to be vice versa.");
      }
      checkConsistency(objectMapSmallerOnes, o, o2);
    }
  }
}

/**
 * @param keyMapValues 
 * @param key 
 * @param <Key> 
 * @param <Value> 
 * @return List<Value>
 */ 
public static <Key, Value> List<Value> getListSafely(Map<Key, List<Value>> keyMapValues, Key key) {
  List<Value> values = keyMapValues.get(key);

  if (values == null) {
    keyMapValues.put(key, values = new LinkedList<Value>());
  }

  return values;
}

/**
 * @author Oku
 *
 * @param <T>
 */
public interface IPairIteratorCallback<T> {
  /**
   * @param o1
   * @param o2
   */
  void pair(T o1, T o2);
}

/**
 * 
 * Iterates through each distinct unordered pair formed by the elements of a given iterator
 *
 * @param it
 * @param callback
 */
public static <T> void iterateDistinctPairs(final Iterator<T> it, IPairIteratorCallback<T> callback) {
  List<T> list = Convert.toMinimumArrayList(new Iterable<T>() {

    @Override
    public Iterator<T> iterator() {
      return it;
    }

  });

  for (int outerIndex = 0; outerIndex < list.size() - 1; outerIndex++) {
    for (int innerIndex = outerIndex + 1; innerIndex < list.size(); innerIndex++) {
      callback.pair(list.get(outerIndex), list.get(innerIndex));
    }
  }
}
Martin
quelle
Rufen Sie einfach checkConsitency metohod mit Parameterliste und Komparator auf.
Martin
Ihr Code wird nicht kompiliert. Klassen Compare, Convert(und möglicherweise andere) sind nicht definiert. Bitte aktualisieren Sie das Code-Sniplet mit einem in sich geschlossenen Beispiel.
Gili
Sie sollten den Tippfehler beheben checkConsi(s)tencyund alle redundanten @paramDeklarationen entfernen , um den Code besser lesbar zu machen.
Roland Illig
3

In meinem Fall habe ich so etwas gemacht:

if (a.someField == null) {
    return 1;
}

if (b.someField == null) {
    return -1;
}

if (a.someField.equals(b.someField)) {
    return a.someOtherField.compareTo(b.someOtherField);
}

return a.someField.compareTo(b.someField);

Was ich vergessen habe zu überprüfen war, wenn sowohl a.someField als auch b.someField null sind.

jc12
quelle
3

Ich habe dies in einem Code gesehen, in dem die häufig wiederkehrende Überprüfung auf Nullwerte durchgeführt wurde:

if(( A==null ) && ( B==null )
  return +1;//WRONG: two null values should return 0!!!
keesp
quelle
2

Wenn compareParents(s1, s2) == -1danncompareParents(s2, s1) == 1 erwartet wird. Mit Ihrem Code ist es nicht immer wahr.

Speziell wenn s1.getParent() == s2 && s2.getParent() == s1. Es ist nur eines der möglichen Probleme.

Andrii Karaivanskyi
quelle
1

Das Bearbeiten der VM-Konfiguration hat bei mir funktioniert.

-Djava.util.Arrays.useLegacyMergeSort=true
Rishav Roy
quelle
Bitte überprüfen Sie noch einmal, ob mein Versuch, Ihnen bei der Formatierung zu helfen, zu nichts geführt hat. Ich bin mir nicht sicher, -wie die vorgeschlagene Lösung beginnen soll. Vielleicht wollten Sie stattdessen so etwas wie eine Aufzählungsliste mit einem Punkt.
Yunnosch
2
Erklären Sie bitte auch, wie dies bei dem beschriebenen Problem hilft. Derzeit handelt es sich praktisch nur um eine Code-Antwort.
Yunnosch
0

Sie können Objektdaten nicht wie s1.getParent() == s2folgt vergleichen : - Dadurch werden die Objektreferenzen verglichen. Sie sollten die equals functionFoo-Klasse überschreiben und sie dann so vergleichens1.getParent().equals(s2)

erimerturk
quelle
Nein, eigentlich denke ich, dass OP versucht, eine Liste zu sortieren, und tatsächlich Referenzen vergleichen möchte.
Edward Falk