1
Vote

CompareTo bug

description

There a serious bug in the pattern used for CompareTo throughout the project. Using (and simplying) GeoCoding as an example, the CompareTo looks like this:
 
            int result  = A.Latitude.CompareTo(B.Latitude);
            result      = result | A.Longitude.CompareTo(B.Longitude);
            return result;
 
This pattern determines that A > B only if every property of A is greater than the corresponding property in B. If any property in A is less than B, than overall A < B.
 
The problem is that it's not reflexive. For example:
NewYork = {Latitude = 40.716, Longitude = -74.0}
Chicago = {Latitude = 41.836, Longitude = -87.684}
 
Now, since NewYork.Latitude < Chicago.Latitude then NewYork < Chicago.
However, since Chicago.Longitude < NewYork.Longitude, then Chicago < NewYork
This could lead to an infinite loop if someone tried sorting nodes.
 
Further, beside fundementally not working, it also has a couple technical problems
  • it assumes that CompareTo returns -1, 0, +1 when it's only required to return <0, 0, >0
  • it requires that every property be tested, even it it's already decided that they are unequal.
     
    A better pattern would be:
     
            int result  = A.Latitude.CompareTo(B.Latitude);
            if (result == 0) result =A.Longitude.CompareTo(B.Longitude);
            return result;

comments

Trickster wrote Jun 27, 2011 at 10:04 PM

I think we can leave only coordinates comparison. Why we need to compare other properties?

JamesCurran wrote Jun 28, 2011 at 1:31 AM

GeoCoding was used just as an example. Every CompareTo throughout the library uses that pattern, and has the same problems. GeoCoding just made for a easy concrete example.

Trickster wrote Jun 28, 2011 at 2:06 AM

Ok, let me reformulate my question. Can we use in CompareTo methods only 'meaningful' properties? Just like Latitude and Longitude in GeoCoding case

JamesCurran wrote Jun 30, 2012 at 1:20 AM

Sorry about takes so long to reply. I never saw the response notification from CodePlex. So, only a year (and a day) later......

Yes, we need only look at meaningful properties (which for most types of nodes, may only be one property)

wrote Feb 14, 2013 at 7:26 PM