Skip to main content

#189 vs #214?

7 replies [Last post]
Anonymous

Okay, probably dumb - but please enlighten me:

- what is the difference between the suggested fixes?
- isn't it wrong to do a c1.compareTo(c2) in the if-branch and a
c2.compareTo(c1) in the else-branch?
- anybody tested what's the performance penalty if we do the compare if
we have two comparables and catch the error if things go wrong?

Jeanette

---------------------------------------------------------------------
To unsubscribe, e-mail: jdnc-unsubscribe@jdnc.dev.java.net
For additional commands, e-mail: jdnc-help@jdnc.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
elkner
Offline
Joined: 2003-06-10
Points: 0

> - what is the difference between the suggested fixes?

Wrt. Sorter.java: none - sorry for submitting it twice (but I'm not getting younger ;-))

> - isn't it wrong to do a c1.compareTo(c2) in the
> if-branch and a
> c2.compareTo(c1) in the else-branch?

Tribute to the Comparable - both may have implemented the Comparable IF but may not live in the same hierarchy/level - so TypeCastException provisioning

> - anybody tested what's the performance penalty if we
> do the compare if
> we have two comparables and catch the error if things
> go wrong?

In 1.5 it is simply "wrong" to compare two Comparables of different types ...

Kleopatra

jdnc-interest@javadesktop.org wrote:
>
>>- isn't it wrong to do a c1.compareTo(c2) in the
>>if-branch and a
>>c2.compareTo(c1) in the else-branch?
>
>
> Tribute to the Comparable - both may have implemented
> the Comparable IF but may not live in the same hierarchy/level
> - so TypeCastException provisioning

should have been more clear - the sign isn't correct: assuming c1 < c2
then c1.compareTo(c2) returns -1, c2.compareTo(c1) returns 1. What we
want is the former (or am I too dizzy from too long hours before the
screen?).

Do you have any example hitting the else branch? Preferably a test
method that's failing with the current implementation. IMO the base
implementation should be as straightforward as possible - if the content
is widely mixed then it can't be handled satifactoryly in the general
context anyway and it's up to the developer to add a comparator which
handles her very special case. Open to arguments, of course (and
_tested_ patches )

Jeanette

---------------------------------------------------------------------
To unsubscribe, e-mail: jdnc-unsubscribe@jdnc.dev.java.net
For additional commands, e-mail: jdnc-help@jdnc.dev.java.net

elkner
Offline
Joined: 2003-06-10
Points: 0

> should have been more clear - the sign isn't correct:
> assuming c1 < c2
> then c1.compareTo(c2) returns -1, c2.compareTo(c1)
> returns 1.

Yep.

> Do you have any example hitting the else branch?

Yes - attached at the end (hope, nobody accusses me of spamming) ;-)

> Preferably a test
> method that's failing with the current
> implementation. IMO the base
> implementation should be as straightforward as
> possible - if the content
> is widely mixed then it can't be handled
> satifactoryly in the general
> context anyway and it's up to the developer to add a
> comparator which
> handles her very special case.

OK. The patch simply avoided to run into an exception -> so one might argue a kind of "cleaner". I did a simple test on my 2 GHz AMD 2800+ and it shows that the introspection needs about 100 nano seconds.

> Open to arguments, of
> course (and
> _tested_ patches )

You caught me 8-(

Regards,
jens.

---schnipp---
package swingx;

public class CompTest {

public class Car implements Comparable {
float speed = 100;
public Car(float speed) { this.speed = speed; }
public int compareTo(Car o) {
return speed < o.speed ? -1 : speed > o.speed ? 1 : 0;
}
}
public class Porsche extends Car {
boolean hasBridgeStone = true;
public Porsche(float speed) { super(speed); }
public int compareTo(Car o) {
if (o instanceof Porsche) {
return ((Porsche) o).hasBridgeStone ? 0 : 1;
}
return super.compareTo(o);
}
}
public class Tractor implements Comparable {
float speed = 20;
public Tractor(float speed) { this.speed = speed; }
public int compareTo(Tractor o) {
return speed < o.speed ? -1 : speed > o.speed ? 1 : 0;
}
}

public int compareOld(Object o1, Object o2) {
if (o1 == null && o2 == null) {
return 0;
}
else if (o1 == null) {
return -1;
}
else if (o2 == null) {
return 1;
}

if ((o1 instanceof Comparable) && (o2 instanceof Comparable)) {
Comparable c1 = (Comparable) o1;
Comparable c2 = (Comparable) o2;
try {
return c1.compareTo(c2);
} catch (ClassCastException ex) {
System.err.println("type mismatch");
}
}

return o1.toString().compareTo(o2.toString());
}

public int compareNew(Object o1, Object o2) {
if (o1 == null) {
return (o2 == null) ? 0 : -1;
} else if (o2 == null) {
return 1;
}

if ((o1.getClass().isInstance(o2)) && (o1 instanceof Comparable)) {
Comparable c1 = (Comparable) o1;
Comparable c2 = (Comparable) o2;
return c1.compareTo(c2);
} else if (o2.getClass().isInstance(o1) && (o2 instanceof Comparable)) {
Comparable c1 = (Comparable) o1;
Comparable c2 = (Comparable) o2;
return -c2.compareTo(c1);
}

return o1.toString().compareTo(o2.toString());
}

public void test(Object a, Object a1) {
long t0 = System.currentTimeMillis();
for (int i=0; i < 1000000; i++) {
compareOld(a,a1);
}
long t1 = System.currentTimeMillis();
System.out.println("Time Old (ms): " + (t1 - t0));
t0 = System.currentTimeMillis();
for (int i=0; i < 1000000; i++) {
compareNew(a,a1);
}
t1 = System.currentTimeMillis();
System.out.println("Time New (ms): " + (t1 - t0));
}

public static void main(String[] args) {
CompTest t = new CompTest();
Car car = t.new Car(180);
Porsche p = t.new Porsche(100.1f);
Tractor m = t.new Tractor(10.2345f);
System.out.println("car.compareTo(p)" + t.compareOld(car, p));
System.out.println("p.compareTo(car)" + t.compareOld(p, car));
System.out.println("m.compareTo(p)" + t.compareOld(m, p));
System.out.println("p.compareTo(m)" + t.compareOld(p, m));
System.out.println("car.compareTo(p)" + t.compareNew(car, p));
System.out.println("p.compareTo(car)" + t.compareNew(p, car));
System.out.println("m.compareTo(p)" + t.compareNew(m, p));
System.out.println("p.compareTo(m)" + t.compareNew(p, m));
for (int i=0; i <5; i++) {
t.test(car, p);
t.test(p, car);
}
}
}
---schnapp---

Kleopatra

jdnc-interest@javadesktop.org wrote:
>
>>Do you have any example hitting the else branch?
>
>
> Yes - attached at the end (hope, nobody accusses me of spamming) ;-)

I asked for it - so by definition it's not ;-)

>
> OK. The patch simply avoided to run into an exception
> -> so one might argue a kind of "cleaner".

yeah - is it logically complete (too lazy to really think about it...)?

> I did a simple test on my 2 GHz AMD 2800+ and
> it shows that the introspection needs about 100 nano seconds.
>

I'm not exactly known to care much about performance - and notoriously
wrong in guessing (would have expected the exception handling to be more
costly). But I tend to go for the simplest possible solution: which is
to assume that the content is more or less comparable and if not leave
it to the developer to do better (with a custom comparator) - the
outcome of _any_ general approach is relatively arbitrary if they don't
fit. Hmm... still undecided

Thanks for the wonderfully weird example - there's an interactive table
test method to see it.

Jeanette

---------------------------------------------------------------------
To unsubscribe, e-mail: jdnc-unsubscribe@jdnc.dev.java.net
For additional commands, e-mail: jdnc-help@jdnc.dev.java.net

elkner
Offline
Joined: 2003-06-10
Points: 0

[Grrr - need to reply a 2nd time, because the first reply was somehow ignored/chomped by the forum software - so this time the short form:]

> yeah - is it logically complete (too lazy to really
> think about it...)?

- think so

> I'm not exactly known to care much about performance
> - and notoriously
> wrong in guessing (would have expected the exception
> handling to be more
> costly).
- shown test tests the best case wrt. your variant, only.
- insert t.test(m,p), delete syserr statement in the exception and run again
- exception handling seems to need ~ 11 µs on 2 GHZ Athlon 2800+ and about 39 µs on ~600 MHz PIII per comparison , so in general about factor 55/110 slower than introspec

> But I tend to go for the simplest possible
> solution: which is
> to assume that the content is more or less comparable
> and if not leave
> it to the developer to do better (with a custom
> comparator) - the
> outcome of _any_ general approach is relatively
> arbitrary if they don't
> fit. Hmm... still undecided

- both variants seem not to make a noticable big difference, so per gusto decision is probably ok

> Thanks for the wonderfully weird example - there's an

- np, wasn't that hard to write it ;-)

> interactive table
> test method to see it.

- okidoki

Regards,
jens.

kleopatra
Offline
Joined: 2003-06-11
Points: 0

Hi Jens,

I decided to take your patch, thanks!

Jeanette

Anonymous

Jeanette,

> - isn't it wrong to do a c1.compareTo(c2) in the
> if-branch and a
> c2.compareTo(c1) in the else-branch?

after carefully reading the Comparable interface description i don't see anything wrong here. It is absolutely the same including that compareTo(null) should throw NPE as if null.compareTo() is being called. The only thing that being hurt is the code consistency, but it also might not be so, if order of comparing demonstrates some inner logic of the code. Hard to judge, but i don't see no crime here. :)