12

I 'm integrating a geographic coordinate class from CodePlex to my personal "toolbox" library. This class uses float fields to store latitude and longitude.

Since the class GeoCoordinate implements IEquatable<GeoCoordinate>, I habitually wrote the Equals method like so:

public bool Equals(GeoCoordinate other)
{
    if (other == null) {
        return false;
    }

    return this.latitude == other.latitude && this.longitude == other.longitude;
}

At this point I stopped and considered that I 'm comparing floating point variables for equality, which is generally a no-no. My thought process then went roughly as follows:

  1. I can only imagine setting the Latitude and Longitude properties once, which means that there will be no errors being accumulated to mess up my comparisons.

  2. On the other hand, it's possible (albeit pointless) to write

    var geo1 = new GeoCoordinate(1.2, 1.2);
    var geo2 = new GeoCoordinate(1.2, 1.2);
    
    // geo1.Equals(geo2) will definitely be true, BUT:
    
    geo2.Latitude *= 10;
    geo2.Latitude /= 10;
    
    // I would think that now all bets are off
    

    Of course this is not something I can imagine doing, but if the public interface of the class allows it then Equals should be able to handle it.

  3. Comparing for equality using a difference < epsilon test would solve the problem of comparing two instances, but create more problems:

    • How to make equality transitive? It sounds impossible.
    • How to produce the same hash code for all values that would compare equal?

      Let's say that epsilon = 0.11 (random example). It follows that GeoCoordinate { 1, 1 } would need the same hash code as GeoCoordinate { 1.1, 1.1 }. But the latter would need the same hash code as GeoCoordinate { 1.2, 1.2 }. You can see where this is going: all instances would need to have the same hash code.

  4. A solution to all of this would be to make GeoCoordinate an immutable class. This would also solve the GetHashCode problem: it is based on latitude and longitude (what else), and if these are are mutable then using GeoCoordinate as a key into a dictionary is asking for trouble. However, to make the class immutable has its own drawbacks:

    • You cannot instantiate-and-configure instances of the class (the WPF paradigm), which might be a pain in some cases
    • Serialization would probably also become a pain due to the loss of a parameterless constructor (I 'm not a .NET serialization expert so that's as much detail as I see here)

Which approach would you suggest? It's easy to make the class fit the requirements I have right now (just make it immutable), but is there some better way?

Edit: I added an item 3 in the list above, shifting the previous item 3 to position 4.

Solution

I 'm going to allow some more time for feedback, but presently I 'm going with the immutable approach. The struct (because that's what it is now) with the relevant members can be seen here; comments and suggestions more than welcome.

Jon
  • 428,835
  • 81
  • 738
  • 806
  • 2
    See also http://stackoverflow.com/questions/6151625/should-i-use-a-struct-or-a-class-to-represent-a-lat-lng-coordinate – ccellar Jul 21 '11 at 15:47
  • @ckeller: Thanks for the link, I hadn't managed to find it. Skeet's solution is pretty much exactly what I have in the PasteBin snippet above, and also what the answers suggest. Should be more than adequate. – Jon Jul 21 '11 at 16:04

4 Answers4

6
  • Equals is mainly used in dictionaries, so the guideline that you should compare floats only with an epsilon does not apply here. Do not attempt to put epsilon logic into Equals. It's the users job to do that as he needs. It's relatively easy to prove that the only hash function that's consistent with epsilon-comparisons it the constant hashfunction.

  • Your implementation has one problem though: It doesn't handle NaNs. You need to use Equals instead of == on the individual coordinates for your Equals method.

    public bool Equals(GeoCoordinate other)
    {
        if (other == null) {
            return false;
        }
    
        return this.latitude.Equals( other.latitude) && this.longitude.Equals(other.longitude);
    }
    
  • The "Don't compare using == but using epsilon" guideline is for the consuming code, not for the implementing code. So I'd implement a function that returns the distance between two geo-coordinates and tell the user to use this for his epsilon comparisons.

  • I'd definitely make it immutable (not sure if a struct or class). It has value semantics and thus should be immutable.
    I usually use something like Linq-to-Xml/Linq-to-Json for serialization, since this allows me to transform the representation between my in-memory model and the on-disk model.
    But you're right that many serializers don't support non default constructors. I regard that as a big flaw in those serializers, not as a flaw in my model. Some serializers simply access the private setters/fields, but personally I think that stinks.

CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
6

A "location" with longitude/latitude to me falls quite nicely into the "immutable value" slot. The position itself doesn't change - if you change the latitude that is a different position. From there, it could be a struct; for float the struct would be the same size as an x64 reference anyway, so no real down side.

Re equality; if the position isn't quite the same, it isn't "equals", at least in the "key" perspective, so I'd be happy with == here. You could add a "is within (x) distance" method if it helped. Of course, great-arc geometry isn't exactly totally free either ;p

Thoughts though:

  • it should override bool Equals(object) as well as adding a bool Equals(GeoCoordinate)
  • it should override GetHashCode() and implement IEquatable<GeoCoordinate>
  • static operators are a nice-to-have optional extra
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • Thanks for the input Marc. The more I think of it the more obvious (famous last words) it becomes that it should be an immutable type, especially if you consider that the class was already fundamentally broken so you can chalk its mutable aspect off as another design error. Also, `IEquatable` is implemented (it was not in the original class), and as for the `Equals(object)` and `GetHashCode` overrides that goes without saying. – Jon Jul 21 '11 at 11:45
2

I would just use long integers instead of floating-point numbers in the underlying model of latitude and longitude. A millisecond of degree is in any case less than two inches, which should be enough detail: store your coordinates in milliseconds and it's simpler, cleaner and bug-proof.

Mauro Vanetti
  • 424
  • 1
  • 5
  • 18
  • 1
    I agree that a fixed point representation feels conceptually cleaner. One problem is that trigonometric functions are not defined on integers, and you need those quite often. – CodesInChaos Jul 21 '11 at 11:08
  • CodeInChaos's comment is spot on. Also, as the question states I have "inherited" the implementation (which looks OK) exactly so that I wouldn't have to write it from scratch. So while your suggestion might be good in itself, it's not a good idea for me to use it. – Jon Jul 21 '11 at 11:32
  • I see, Jon, I didn't get that point. @CodeInChaos I would definitely have a method to get a float version in degrees of the coordinate (divide by 3600000) for trigonometric stuff, but keep the integers as the underlying model for exact comparison, North-South or East-West distances etc. – Mauro Vanetti Jul 21 '11 at 13:24
1

Depending on your use of the lat/lon struct, I would be concerned about using floats instead of doubles for lat/lon. For example, if you are doing real-time integration of lat/lon, then you will need double precision. This is because a degree is 1 nautical mile and depending on the time step of the integration, you are only moving a very small amount per time iteration.

For doing the equality test, I would use a simple distance formula based on a equirectangular approximation and decide that if the other point was within my decided tolerance (an inch or two), then declare them equal. The equirectangular approximation I would use is given here.

TreyA
  • 3,339
  • 2
  • 20
  • 26
  • The problem with doing equality that way is that it cannot be made transitive. The exact same problem manifests itself (not so catastrophically, but it's much more easily seen) in correctly implementing `GetHashCode` -- so I 've ruled that approach out. – Jon Jul 21 '11 at 16:23
  • I have to admit I am a bit lost in the programming jargon. But, to harp on the float vs double issue: I am assuming your float is IEEE754 32-bit which I believe only has 7 significant digits. For accurate lat/lon (at least in aviation) you need at least 10 significant figures (consider lon -179.1234567). – TreyA Jul 22 '11 at 15:21