"Metode perbandingan melanggar kontrak umum!"

187

Dapatkah seseorang menjelaskan kepada saya secara sederhana, mengapa kode ini memberikan pengecualian, "Metode perbandingan melanggar kontrak umum!", Dan bagaimana cara memperbaikinya?

private int compareParents(Foo s1, Foo s2) {
    if (s1.getParent() == s2) return -1;
    if (s2.getParent() == s1) return 1;
    return 0;
}
n00bster
sumber
1
Apa nama dan kelas Pengecualian? Apakah ini IllegalArgumentException? Jika saya harus menebak saya akan berpikir bahwa Anda harus melakukan s1.getParent().equals(s2)bukan s1.getParent() == s2.
Freiheit
Dan pengecualian itu terlempar juga.
Matthew Farwell
2
Saya tidak tahu banyak tentang Java atau tentang API perbandingan Java, tetapi metode perbandingan ini tampaknya salah besar. Misalkan s1orang tua s2, dan s2bukan orang tua dari s1. Kemudian compareParents(s1, s2)adalah 0, tapi compareParents(s2, s1)adalah 1. Itu tidak masuk akal. (Selain itu, ini bukan transitif, seperti aix yang disebutkan di bawah ini.)
mqp
4
Kesalahan ini tampaknya hanya dihasilkan oleh perpustakaan tertentu cr.openjdk.java.net/~martin/webrevs/openjdk7/timsort/src/share/…
Peter Lawrey
dalam java, Anda bisa menggunakan equals (return a boolean) atau compareTo (return -1, 0 atau +1). Timpa fungsi ini di kelas Foo Anda dan setelah itu, Anda dapat memeriksa s1.getParent (). Sama dengan (s2) ...
Mualig

Jawaban:

261

Komparator Anda tidak transitif.

Biarkan Amenjadi orang tua dari B, dan Bmenjadi orang tua dari C. Sejak A > Bdan B > C, maka harus demikian A > C. Namun, jika pembanding Anda diaktifkan Adan C, itu akan mengembalikan nol, artinyaA == C . Ini melanggar kontrak dan karenanya melempar pengecualian.

Itu agak baik dari perpustakaan untuk mendeteksi ini dan memberi tahu Anda, daripada berperilaku tidak menentu.

Salah satu cara untuk memenuhi persyaratan transitivitas compareParents()adalah dengan melintasi getParent()rantai alih-alih hanya melihat leluhur langsung.

NPE
sumber
3
Diperkenalkan di Java 7's java.util.Arrays.sort stackoverflow.com/questions/7849539/…
leonbloy
46
Fakta perpustakaan mendeteksi ini luar biasa. Seseorang di bawah sinar matahari pantas untuk membuang raksasa Terima kasih .
Qix - MONICA DISEBUTKAN
Bisakah Anda menggeneralisasi jawaban ini sedikit agar pertanyaan ini lebih bermanfaat sebagai posting referensi?
Bernhard Barker
1
@Qix - sebanyak yang saya suka Sun, ini ditambahkan di Java 7 di bawah bendera Oracle
isapir
1
@ mengisapir Sial! Tangkapan yang bagus.
Qix - MONICA DISALAHKAN
38

Hanya karena ini yang saya dapatkan ketika saya mencari kesalahan ini di Google, masalah saya adalah yang saya miliki

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

yang value >= other.valueharus (jelas) benar-benar menjadi value > other.valuesehingga Anda dapat benar-benar kembali 0 dengan objek yang sama.

kamu786
sumber
7
Saya harus menambahkan bahwa jika salah satu dari Anda valueadalah NaN (jika valueadalah doubleatau float), itu akan gagal juga.
Matthieu
22

Pelanggaran kontrak seringkali berarti bahwa pembanding tidak memberikan nilai yang benar atau konsisten ketika membandingkan objek. Misalnya, Anda mungkin ingin melakukan perbandingan string dan memaksa string kosong untuk mengurutkan sampai akhir dengan:

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

Tapi ini mengabaikan kasus di mana KEDUA satu dan dua kosong - dan dalam kasus itu, nilai yang salah dikembalikan (1 bukannya 0 untuk menunjukkan kecocokan), dan pembanding melaporkan itu sebagai pelanggaran. Seharusnya ditulis sebagai:

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

Bahkan jika compareTo Anda memegang transitivitas dalam teori, kadang-kadang bug halus mengacaukan segalanya ... seperti kesalahan aritmatika floating point. Itu terjadi pada saya. ini kode saya:

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;

}   

Properti transitif jelas berlaku, tetapi untuk beberapa alasan saya mendapatkan IllegalArgumentException. Dan ternyata karena kesalahan kecil dalam aritmatika floating point, kesalahan pembulatan di mana menyebabkan properti transitif pecah di mana mereka seharusnya tidak! Jadi saya menulis ulang kode untuk mempertimbangkan perbedaan yang sangat kecil 0, dan berhasil:

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
sumber
2
Ini sangat membantu! Kode saya secara logis oke tetapi ada kesalahan karena masalah presisi.
JSong
6

Dalam kasus kami mendapatkan kesalahan ini karena kami tidak sengaja membalik urutan perbandingan s1 dan s2. Jadi hati-hati untuk itu. Itu jelas jauh lebih rumit daripada yang berikut tetapi ini adalah ilustrasi:

s1 == s2   
    return 0;
s2 > s1 
    return 1;
s1 < s2 
    return -1;
Paman Iroh
sumber
3

Java tidak memeriksa konsistensi dalam arti yang ketat, hanya memberi tahu Anda jika mengalami masalah serius. Juga tidak memberi Anda banyak informasi dari kesalahan.

Saya bingung dengan apa yang terjadi di sorter saya dan membuat konsistensi yang ketatChecker, mungkin ini akan membantu Anda:

/**
 * @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
sumber
Cukup panggil metohod checkConsitency dengan daftar parameter dan pembanding.
Martin
Kode Anda tidak dikompilasi. Kelas Compare, Convert(dan berpotensi lainnya) tidak didefinisikan. Harap perbarui sniplet kode dengan contoh lengkap.
Gili
Anda harus memperbaiki kesalahan ketik checkConsi(s)tencydan menghapus semua @paramdeklarasi berlebihan untuk membuat kode lebih mudah dibaca.
Roland Illig
3

Dalam kasus saya, saya melakukan sesuatu seperti berikut:

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

Yang saya lupa periksa adalah ketika a.someField dan b.someField keduanya nol.

jc12
sumber
3

Saya telah melihat ini terjadi dalam sepotong kode di mana pemeriksaan berulang untuk nilai null dilakukan:

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

Jika compareParents(s1, s2) == -1kemudian compareParents(s2, s1) == 1diharapkan. Dengan kode Anda itu tidak selalu benar.

Khususnya jika s1.getParent() == s2 && s2.getParent() == s1. Itu hanya salah satu masalah yang mungkin terjadi.

Andrii Karaivanskyi
sumber
1

Mengedit Konfigurasi VM berfungsi untuk saya.

-Djava.util.Arrays.useLegacyMergeSort=true
Rishav Roy
sumber
Harap periksa kembali bahwa upaya saya untuk membantu Anda dengan pemformatan tidak merusak apa pun. Saya tidak yakin tentang -a awal solusi yang diusulkan. Mungkin Anda bermaksud sesuatu seperti daftar poin-poin satu item saja.
Yunnosch
2
Tolong jelaskan juga bagaimana hal ini membantu masalah yang dijelaskan. Saat ini praktis jawaban hanya kode.
Yunnosch
0

Anda tidak dapat membandingkan data objek seperti ini: s1.getParent() == s2- ini akan membandingkan referensi objek. Anda harus mengganti equals functionuntuk kelas Foo dan kemudian membandingkannya seperti inis1.getParent().equals(s2)

erimerturk
sumber
Tidak, sebenarnya saya pikir OP sedang mencoba menyortir daftar, dan ingin membandingkan referensi.
Edward Falk