5
\$\begingroup\$

I'm working on some book material on classes and interfaces in F#. My example is a latitude/longitude class:

    open System

    type LatLon(latitude : float, longitude : float) =

        member __.Latitude = latitude
        member __.Longitude = longitude

        override this.GetHashCode() =
            hash (this.Latitude, this.Longitude)

        override this.Equals(thatObj) =
            match thatObj with
            | :? LatLon as that -> 
                this.Latitude = that.Latitude
                && this.Longitude = that.Longitude    
            | _ -> false

        interface IEquatable<LatLon> with
            member this.Equals(that : LatLon) =
                this.Equals(that)   

        interface IComparable with
            member this.CompareTo(thatObj) =
                match thatObj with
                | :? LatLon as that -> 
                    compare (this.Latitude, this.Longitude) (that.Latitude, that.Longitude)                    
                | _ -> 
                    raise <| ArgumentException("Can't compare instances of different types")

    let landsEnd = LatLon(50.07, -5.72)
    let johnOGroats = LatLon(58.64, -3.07)
    let landsEnd2 = LatLon(50.07, -5.72)

    let demo() =
        // false
        printfn "%b" (landsEnd = johnOGroats)
        // true
        printfn "%b" (landsEnd = landsEnd2)

        // 50.070000, -5.720000
        // 58.640000, -3.070000
        [ landsEnd; johnOGroats; landsEnd2 ] 
        |> Set.ofList
        |> Seq.iter (fun ll -> printfn "%f, %f" ll.Latitude ll.Longitude)

By the way, I'm well aware that in this case, a record type would give the necessary structural equality out of the box. But the material I want to cover with this example is specifically about classes.

My main concerns are:

  • The code works the same if I don't implement IEquatable, as it uses the Equals() override. Is it considered desirable to implement IEquatable anyway, and/or does it have advantages I haven't spotted?

  • What about the (currently very-lightly-documented) op_Equality? Should that be used as well/instead?

  • The code works fine (for example Set construction, requiring comparison) if I implement IComparable but not IComparable<'T>. Have I missed out on anything useful by not using IComparable<'T>?

Obviously I'm amenable to any other feedback on the quality/style of this code.

Many thanks!

\$\endgroup\$
5
\$\begingroup\$

There's a suggestion for [<AllowNullLiteral>]: I think this is a bad idea.

You wrote a class that is a lat/long pair, as a result, it's a standard GeoCoordinate, which is a regular value-type, and should be marked as such. I get that the book calls it a class, but as an F# developer, how many times in your life have you actually had to think about the difference?

As a result, I'd add the structure definition in some manner, either via struct/end, or [<StructAttribute>].

I also recommend implementing the generic IComparable<T>. It's cheap, and can offer more performance to avoid boxing/unboxing should the caller need it. (Reed also covered this.) Some of the newer API's will try to rely on both, preferring the IComparable<T>. You may also want to read this answer to another question, as it talks about a lot of performance intricacies with regard to the standard BCL stuff. (You may want to create an IEqualityComparer<T>, for example.)

Some might argue that the guidelines say to keep the instance size "under 16 bytes", I personally think that's rubbish, as I'm pretty sure that should read "at or under 16 bytes". These are not always law.

The .NET BCL contradicts itself here several times (Size structure, Guid structure, too lazy to find the others), and the Struct Design guidelines don't mention it, only the class vs. struct guidelines.

As a result, I think you're safe in that regard here. The item you're talking about is small (16-bytes), represents a value (physical location that doesn't change), and should not be directly changed. Just as well, the copying performance can often use native SIMD instructions, so it should not be a big deal to copy 16-bytes. (I had a source on this, but I lost it, going to try to dig it up again.)

\$\endgroup\$
4
\$\begingroup\$

First off, given that this is about using classes, any real world scenario is likely going to do this because of interop with other languages (C#/VB/etc). Given that, I'd recommend adding [<AllowNullLiteral>], since usage from C# will likely result in null values polluting the usage of this type. You'd then need to write defensively around that.

The code works the same if I don't implement IEquatable, as it uses the Equals() override. Is it considered desirable to implement IEquatable anyway, and/or does it have advantages I haven't spotted?

Yes, it can be advantageous to do this, as it (can) avoid the boxing if written properly. In this case, you're delegating to your Equals method, so you lose that advantage.

If you make an Equals method that doesn't box, and have both implementations delegate to it, you gain back that advantage in many scenarios.

Normally, this is simple in C# - you'd use the implicit implementation of the interface for the main method, then override Object.Equals to call it. In F#, this is slightly less simple. I typically end up writing something like:

[<AllowNullLiteral>] // Note this added, since it'll come this way from C#/VB anyways
type LatLon(latitude : float, longitude : float) =
    let eq (that : LatLon) = 
        if isNull that then
            false
        else
            latitude = that.Latitude && longitude = that.Longitude        
// ... other code unchanged
    override this.Equals(thatObj) = 
        match thatObj with
        | :? LatLon as that -> eq that
        | _ -> false

    interface IEquatable<LatLon> with
        member this.Equals(that : LatLon) = eq that

What about the (currently very-lightly-documented) op_Equality? Should that be used as well/instead?

This can be useful for interop from C# or VB, as it will allow == to work from C#, etc.

The code works fine (for example Set construction, requiring comparison) if I implement IComparable but not IComparable<'T>. Have I missed out on anything useful by not using IComparable<'T>?

Mainly performance issues due to unnecessary boxing which will occur in many scenarios.

\$\endgroup\$
  • \$\begingroup\$ I disagree with [<AllowNullLiteral>] very heavily: this looks like a standard value-type / struct, and should remain as-is there. If anything, OP should apply [<StructAttribute>] because it should be handled like a struct in every manner. (Everything else you wrote is great.) \$\endgroup\$ – Der Kommissar Jun 18 '18 at 20:07
  • \$\begingroup\$ @202_accepted Struct usage might make sense, might not, depending on how it's going to be used and the semantics you want. If you're staying all F#, I'd just use a record. \$\endgroup\$ – Reed Copsey Jun 18 '18 at 20:08
  • \$\begingroup\$ To be honest, I'm mixed on the AllowNull - In an interop scenario, I find it useful as it forces me to think about null when using the type, since I know it's always a possibility whether or not that type is flagged. \$\endgroup\$ – Reed Copsey Jun 18 '18 at 20:10
  • \$\begingroup\$ But OP suggested they want a regular class, being a true value, I'd suspect that OP should use the struct definition in some manner. This is especially useful in C#/VB.NET interop. Yes, if all F# a record is the right way to go, but if you're building it as a class like this to begin with, chances are it's not all F#. ;) \$\endgroup\$ – Der Kommissar Jun 18 '18 at 20:11
  • \$\begingroup\$ @202_accepted Struct in C# interop is not something I recommend unless there's a good reason to do so. It tends to complicate usage, and can lead to perf issues, especially if being stored in dictionaries, etc. C# defaults to reference types, and the fact they are "using as a class" suggests reference type semantics were desired. \$\endgroup\$ – Reed Copsey Jun 18 '18 at 20:13
3
\$\begingroup\$

Don’t compare floats!

In Praxis there are only a few cases where spatial coordinates can be compared directly. In my 3D geometry code I even have an explicit NoComparison attribute on all my classes for points or vectors. Comparing floats should always be done with a tolerance value that is appropriate for your current domain. In case of Longitude and Latitude that tolerance would probably be one meter, or much more if you work with poor GPS signals.

For a book I would pick an example based on integers or strings. Where you don’t have to deal with all the problems of floats.

See http://floating-point-gui.de and https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

\$\endgroup\$
  • \$\begingroup\$ Interesting thought, but I will disagree a bit. Although I agree that comparing floating point numbers is tricky and using some epsilon (maximal difference) is usefull in some cases, it is not for IEquatable and such, because you can then get (a+e).Equals(a) and (a-e).Equals(a) but !(a-e).Equals(a+e) which is a problem (non-transitive operator). And specifically for Lat/Lon, I would use 1e-7*Math.Round (1e7*value) in the constructor (the 1e-7 is precision of uBlox GPS which uses FIXED point numbers ... which is another alternative) \$\endgroup\$ – firda Jun 19 '18 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.