• Post Reply Bookmark Topic Watch Topic
  • New Topic

Whats wrong with this Comparator?  RSS feed

 
Susan
Ranch Hand
Posts: 54
  • Mark post as helpful
  • send pies
  • 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);
}
}
 
Stan James
(instanceof Sidekick)
Ranch Hand
Posts: 8791
  • Mark post as helpful
  • send pies
  • 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 ]
 
Stefan Wagner
Ranch Hand
Posts: 1923
Linux Postgres Database Scala
  • Mark post as helpful
  • send pies
  • 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 ...
 
Roy Simon
Ranch Hand
Posts: 62
  • Mark post as helpful
  • send pies
  • 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
 
Garrett Rowe
Ranch Hand
Posts: 1296
  • Mark post as helpful
  • send pies
  • 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 ]
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!