• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Whats wrong with this Comparator?

 
Ranch Hand
Posts: 54
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Hi,

can anybody see anything wrong with my comparator below? MyObject does not override equals or hashCode.

public class MyComparator implements Comparator {

public MyComparator() {}

public int compare(Object o1, Object o2) {
MyObject obj1 = (MyObject)o1;
MyObject obj2 = (MyObject)o2;

if ( obj1.equals(obj2)) {
return 0;
}

if (obj2.getDate() == null) {
return -1;
} else if (obj1.getDate() == null) {
return 1;
} else if (obj2.getDate().equals(obj1.getDate())) {
if ( obj2.getID() == null ) {
return -1;
} else if ( obj1.getID() == null ) {
return 1;
} else {
return (obj1.getID().compareTo(obj2.getID()));
}
} else {
return obj1.getDate().compareTo(obj2.getDate());
}
}

public boolean equals(Object obj) {
return this.equals(obj);
}
}
 
(instanceof Sidekick)
Posts: 8791
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
What does it do or not do that qualifies as wrong?

I'd give this another look. Can you trace what line of code will execute when you call this.equals()?


A personal preference is to eliminate "else" after "return". You could turn your compare logic into:

Some folks have fits over multiple returns in a method. I usually agree but I can tune them out by humming loudly in certain methods like this.

We may also get some discussion about sorting nulls rather than throwing NullPointerExceptions. Depending on the business meaning of "null" in your objects this may be just fine.

BTW: Look into the CODE button below the editor. It inserts tags that preserve your code formatting.
[ April 01, 2006: Message edited by: Stan James ]
 
Ranch Hand
Posts: 1923
Scala Postgres Database Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Here is a problem:


Think of two MyObjects, both have getDate() return null.

If you compare (mo1, mo2) you get -1, and if you compare (mo2, mo1) you get -1 too.

The same is true for getID.

The javadocs say:

The ordering imposed by a Comparator c on a set of elements S is said to be consistent with equals if and only if (compare((Object)e1, (Object)e2)==0) has the same boolean value as e1.equals((Object)e2) for every e1 and e2 in S.


You claim not to override equals.
Well. If (o1 != o2) but
((o1.getDate().equals (o2.getDate ())) AND (o1.getId ().equals (o2.getId ()))) o1.equals(o2) will return false, but compareTo (o1, o2) will return 0.

You should override equals and check for
(o1.getDate () == null && o2.getDate () == null) and
..getId ...
 
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

public int compare(Object o1, Object o2) {
MyObject obj1 = (MyObject)o1;
MyObject obj2 = (MyObject)o2;

if ( obj1.equals(obj2)) {
return 0;
}

if (obj2.getDate() == null) {
return -1;
} else if (obj1.getDate() == null) {
return 1;
} else if (obj2.getDate().equals(obj1.getDate())) {
if ( obj2.getID() == null ) {
return -1;
} else if ( obj1.getID() == null ) {
return 1;
} else {
return (obj1.getID().compareTo(obj2.getID()));
}
} else {
return obj1.getDate().compareTo(obj2.getDate());
}
}



Are you comapring three objects??? viz the implicit this, obj1 and obj2??
Why dont you provide an overriden equals and hashcode method for the MyObject class and then all will become easy!!!

Regards
Simon
 
Ranch Hand
Posts: 1296
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Are you comapring three objects??? viz the implicit this, obj1 and obj2??

It looks like that the class that implements Comparator is seperate from the MyObject class. Therefore this would imply that 'this' would be the Comparator. The problem with the method isn't that three objects are being compared (they're not) but not adhering to the compare() contract of the Comparator interface as Stefan pointed out.
[ April 02, 2006: Message edited by: Garrett Rowe ]
 
Consider Paul's rocket mass heater.
reply
    Bookmark Topic Watch Topic
  • New Topic