"La méthode de comparaison viole son contrat général!"

192

Quelqu'un peut-il m'expliquer en termes simples, pourquoi ce code lève-t-il une exception, "La méthode de comparaison viole son contrat général!", Et comment puis-je y remédier?

private int compareParents(Foo s1, Foo s2) {
    if (s1.getParent() == s2) return -1;
    if (s2.getParent() == s1) return 1;
    return 0;
}
n00bster
la source
1
Quel est le nom et la classe de l'exception? S'agit-il d'une exception IllegalArgumentException? Si je devais deviner, je penserais que vous devriez faire s1.getParent().equals(s2)au lieu de s1.getParent() == s2.
Freiheit
Et l'exception qui est lancée aussi.
Matthew Farwell
2
Je ne sais pas grand-chose sur Java ou sur les API de comparaison Java, mais cette méthode de comparaison semble totalement erronée. Supposons que ce s1soit le parent s2et s2non le parent de s1. Alors compareParents(s1, s2)est 0, mais compareParents(s2, s1)est 1. Cela n'a pas de sens. (De plus, ce n'est pas transitif, comme aix mentionné ci-dessous.)
mqp
4
Cette erreur ne semble être produite que par une bibliothèque spécifique cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/src/share/…
Peter Lawrey
en java, vous pouvez utiliser equals (retourne un booléen) ou compareTo (retourne -1, 0 ou +1). Remplacez cette fonction dans votre classe Foo et après cela, vous pouvez vérifier s1.getParent (). Equals (s2) ...
Mualig

Réponses:

266

Votre comparateur n'est pas transitif.

Soit Ale parent de B, et Ble parent de C. Depuis A > Bet B > C, alors il doit en être ainsi A > C. Cependant, si votre comparateur est appelé sur Aet C, il renverra zéro, c'est-à-dire A == C. Cela viole le contrat et jette donc l'exception.

C'est plutôt gentil de la part de la bibliothèque de détecter cela et de vous le faire savoir, plutôt que de se comporter de manière erratique.

Une façon de satisfaire l'exigence de transitivité dans compareParents()est de traverser la getParent()chaîne au lieu de regarder uniquement l'ancêtre immédiat.

NPE
la source
3
Introduit dans java.util.Arrays.sort stackoverflow.com/questions/7849539/… de
leonbloy
46
Le fait que la bibliothèque détecte cela est génial. Quelqu'un au soleil mérite de jeter un géant . De rien .
Qix - MONICA A ÉTÉ MISTREATED
Pouvez-vous généraliser un peu cette réponse pour rendre cette question plus utile comme article de référence?
Bernhard Barker
1
@Qix - autant que j'aime Sun, cela a été ajouté dans Java 7 sous la bannière Oracle
isapir
1
@isapir Merde! Bonne prise.
Qix - MONICA A ÉTÉ BRUYÉ
39

Juste parce que c'est ce que j'ai obtenu lorsque j'ai recherché cette erreur sur Google, mon problème était que j'avais

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

cela value >= other.valuedevrait (évidemment) être en fait value > other.valuepour que vous puissiez réellement renvoyer 0 avec des objets égaux.

vous786
la source
7
Je dois ajouter que si l'un de vos valueest un NaN (si valueest un doubleou float), il échouerait également.
Matthieu
25

La violation du contrat signifie souvent que le comparateur ne fournit pas la valeur correcte ou cohérente lors de la comparaison d'objets. Par exemple, vous pouvez effectuer une comparaison de chaînes et forcer les chaînes vides à trier jusqu'à la fin avec:

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

Mais cela néglige le cas où un et deux sont vides - et dans ce cas, la mauvaise valeur est renvoyée (1 au lieu de 0 pour afficher une correspondance), et le comparateur signale cela comme une violation. Il aurait dû être écrit comme suit:

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
la source
13

Même si votre compareTo tient en théorie la transitivité, des bugs parfois subtils gâchent les choses ... comme l'erreur arithmétique en virgule flottante. Cela m'est arrivé. c'était mon 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;

}   

La propriété transitive tient clairement, mais pour une raison quelconque, j'obtenais l'exception IllegalArgumentException. Et il s'avère qu'en raison de minuscules erreurs d'arithmétique en virgule flottante, les erreurs d'arrondi entraînaient la rupture de la propriété transitive là où elle ne le devrait pas! J'ai donc réécrit le code pour prendre en compte de très petites différences 0, et cela a fonctionné:

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
la source
2
Cela a été utile! Mon code était logiquement correct mais il y avait une erreur à cause du problème de précision.
JSong
6

Dans notre cas, nous recevions cette erreur parce que nous avions accidentellement inversé l'ordre de comparaison de s1 et s2. Alors faites attention à cela. C'était évidemment beaucoup plus compliqué que ce qui suit mais ceci est une illustration:

s1 == s2   
    return 0;
s2 > s1 
    return 1;
s1 < s2 
    return -1;
Oncle Iroh
la source
4

Dans mon cas, je faisais quelque chose comme ce qui suit:

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);

Ce que j'ai oublié de vérifier, c'est quand a.someField et b.someField sont nuls.

jc12
la source
3

Java ne vérifie pas la cohérence au sens strict du terme, vous avertit uniquement s'il rencontre de sérieux problèmes. De plus, cela ne vous donne pas beaucoup d'informations sur l'erreur.

J'ai été intrigué par ce qui se passe dans mon trieur et j'ai fait un contrôle de cohérence strict, peut-être que cela vous aidera:

/**
 * @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
la source
Appelez simplement la méthode checkConsitency avec la liste de paramètres et le comparateur.
Martin
Votre code ne se compile pas. Les classes Compare, Convert(et éventuellement d' autres) ne sont pas définis. Veuillez mettre à jour l'extrait de code avec un exemple autonome.
Gili
Vous devez corriger la faute de frappe checkConsi(s)tencyet supprimer toutes les @paramdéclarations redondantes pour rendre le code plus lisible.
Roland Illig
3

J'ai vu cela se produire dans un morceau de code où la vérification souvent récurrente des valeurs nulles a été effectuée:

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

Si compareParents(s1, s2) == -1alors compareParents(s2, s1) == 1est attendu. Avec votre code, ce n'est pas toujours vrai.

Plus précisément si s1.getParent() == s2 && s2.getParent() == s1. Ce n'est qu'un des problèmes possibles.

Andrii Karaivanskyi
la source
2

La modification de la configuration de la VM a fonctionné pour moi.

-Djava.util.Arrays.useLegacyMergeSort=true
Rishav Roy
la source
Veuillez vérifier que ma tentative de vous aider avec le formatage n'a rien cassé. Je ne suis pas sûr -du début de la solution proposée. Peut-être que vous vouliez plutôt quelque chose comme une liste à puces à un élément.
Yunnosch
2
Veuillez également expliquer comment cela permet de résoudre le problème décrit. Il s'agit actuellement pratiquement d'une réponse codée uniquement.
Yunnosch
0

Vous ne pouvez pas comparer les données d'objet comme ceci: s1.getParent() == s2- cela comparera les références d'objet. Vous devez remplacer la equals functionclasse Foo et les comparer comme cecis1.getParent().equals(s2)

Erimerturk
la source
Non, en fait, je pense qu'OP essaie de trier une liste quelconque et souhaite comparer les références.
Edward Falk