"O método de comparação viola seu contrato geral!"

187

Alguém pode me explicar em termos simples, por que esse código gera uma exceção "O método de comparação viola seu contrato geral!" E como faço para corrigi-lo?

private int compareParents(Foo s1, Foo s2) {
    if (s1.getParent() == s2) return -1;
    if (s2.getParent() == s1) return 1;
    return 0;
}
n00bster
fonte
1
Qual é o nome e a classe da exceção? É uma IllegalArgumentException? Se eu tivesse que adivinhar, pensaria que você deveria estar fazendo em s1.getParent().equals(s2)vez de s1.getParent() == s2.
Freiheit
E a exceção que é lançada também.
Matthew Farwell
2
Não sei muito sobre Java ou sobre APIs de comparação de Java, mas esse método de comparação parece totalmente errado. Suponha que s1é o pai de s2, e s2não é o pai de s1. Então compareParents(s1, s2)é 0, mas compareParents(s2, s1)é 1. Isso não faz sentido. (Além disso, não é transitiva, como aix mencionados abaixo.)
MQP
4
Este erro parece ser produzida apenas por uma biblioteca específica cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/src/share/...
Peter Lawrey
em java, você pode usar iguais (retornar um booleano) ou compareTo (retornar -1, 0 ou +1). Substituir esse funções em sua classe Foo e, depois disso, você pode verificar s1.getParent () é igual a (s2) ....
Mualig

Respostas:

261

Seu comparador não é transitivo.

Let Aser o pai de B, e Bser o pai de C. Desde A > Be B > C, então deve ser o caso A > C. No entanto, se o seu comparador for chamado Ae Cretornará zero, o que significa A == C. Isso viola o contrato e, portanto, gera a exceção.

É bastante agradável da biblioteca detectar isso e informar você, em vez de se comportar de maneira irregular.

Uma maneira de satisfazer o requisito de transitividade compareParents()é atravessar a getParent()cadeia em vez de apenas olhar para o ancestral imediato.

NPE
fonte
3
Introduzido no java.util.Arrays.sort stackoverflow.com/questions/7849539/…
#
46
O fato de a biblioteca estar detectando isso é impressionante. Alguém no sol merece jogar fora um gigante . De nada .
Qix - MONICA FOI ERRADA EM 04/03
Você pode generalizar um pouco essa resposta para tornar essa pergunta mais útil como um post de referência?
Bernhard Barker
1
@Qix - tanto quanto eu amo Sun, este foi adicionado no Java 7 sob o Oráculo bandeira
isapir
1
@isapir Porra! Boa pegada.
Qix - MONICA FOI ERRADA EM
38

Só porque é isso que recebi quando pesquisei esse erro no Google, meu problema era que eu tinha

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

o value >= other.value(obviamente) deve ser realmente, value > other.valuepara que você possa retornar 0 com objetos iguais.

you786
fonte
7
Devo acrescentar que, se algum de vocês valueé um NaN (se valueé um doubleou float), também falharia.
Matthieu
22

A violação do contrato geralmente significa que o comparador não está fornecendo o valor correto ou consistente ao comparar objetos. Por exemplo, convém executar uma comparação de cadeias e forçar cadeias vazias para classificar até o final com:

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

Mas isso ignora o caso em que AMBOS um e dois estão vazios - e, nesse caso, o valor errado é retornado (1 em vez de 0 para mostrar uma correspondência), e o comparador relata isso como uma violação. Deveria ter sido escrito como:

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
fonte
13

Mesmo que seu compareTo mantenha transitividade na teoria, os erros sutis às vezes atrapalham as coisas ... como erro aritmético de ponto flutuante. Isso aconteceu comigo. este foi o meu código:

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;

}   

A propriedade transitiva claramente é válida, mas por algum motivo eu estava recebendo a IllegalArgumentException. E acontece que, devido a pequenos erros na aritmética de ponto flutuante, os erros de arredondamento causavam a quebra da propriedade transitiva onde não deveriam! Então, eu reescrevi o código para considerar realmente pequenas diferenças 0, e funcionou:

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
fonte
2
Isso foi útil! Meu código estava logicamente correto, mas ocorreu um erro devido ao problema de precisão.
JSong 24/09/18
6

No nosso caso, estávamos recebendo esse erro porque acidentalmente invertemos a ordem de comparação de s1 e s2. Então, cuidado com isso. Era obviamente muito mais complicado que o seguinte, mas esta é uma ilustração:

s1 == s2   
    return 0;
s2 > s1 
    return 1;
s1 < s2 
    return -1;
Tio Iroh
fonte
3

O Java não verifica a consistência em sentido estrito, apenas o notifica se encontrar problemas sérios. Também não fornece muitas informações sobre o erro.

Fiquei intrigado com o que está acontecendo no meu classificador e fiz um rigoroso consistencyChecker, talvez isso ajude você:

/**
 * @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
fonte
Simplesmente invoque o método checkConsitency com lista de parâmetros e comparador.
Martin Martin
Seu código não compila. Classes Compare, Convert(e potencialmente outros) não são definidos. Atualize o snippet de código com um exemplo independente.
Gili
Você deve corrigir o erro de digitação checkConsi(s)tencye remover todas as @paramdeclarações redundantes para tornar o código mais legível.
Roland Illig
3

No meu caso, eu estava fazendo algo como o seguinte:

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

O que eu esqueci de verificar foi quando a.someField e b.someField são nulos.

jc12
fonte
3

Eu já vi isso acontecer em um pedaço de código em que a verificação frequentemente recorrente de valores nulos foi realizada:

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

Se compareParents(s1, s2) == -1então compareParents(s2, s1) == 1é esperado. Com o seu código nem sempre é verdade.

Especificamente se s1.getParent() == s2 && s2.getParent() == s1. É apenas um dos possíveis problemas.

Andrii Karaivanskyi
fonte
1

A edição da configuração da VM funcionou para mim.

-Djava.util.Arrays.useLegacyMergeSort=true
Rishav Roy
fonte
Verifique novamente se minha tentativa de ajudá-lo com a formatação não quebrou nada. Não tenho certeza sobre -o início da solução proposta. Talvez você tenha planejado algo como uma lista de marcadores de um item.
Yunnosch 23/07/19
2
Explique também como isso ajuda no problema descrito. Atualmente, é praticamente uma resposta apenas de código.
Yunnosch 23/07/19
0

Você não pode comparar dados de objetos como este: s1.getParent() == s2- isto irá comparar as referências de objetos. Você deve substituir equals functiona classe Foo e compará-las dessa maneiras1.getParent().equals(s2)

erimerturk
fonte
Não, na verdade acho que o OP está tentando classificar uma lista de algum tipo e quer comparar as referências.
Edward Falk