Equals method for a class representing an origin and destination

Have I over-ridden GetHashCode properly in the below code?

public class ODPair
{
    #region Constructors and Destructors

    public ODPair(string origin, string destination)
    {
        if (string.IsNullOrEmpty(origin))
        {
            throw new ArgumentNullException(origin, "Origin of an ODPair should be non-null");
        }

        if (string.IsNullOrEmpty(destination))
        {
            throw new ArgumentNullException(destination, "Destination of an ODPair should be non-null");
        }

        this.Origin = origin;
        this.Destination = destination;
    }

    #endregion

    #region Public Properties

    public string Destination { get; private set; }

    public string Origin { get; private set; }

    #endregion

    #region Public Methods and Operators

    public override bool Equals(object obj)
    {
        if (obj == null)
        {
            return false;
        }

        var pair = obj as ODPair;
        if (pair == null)
        {
            return false;
        }

        return this.Origin == pair.Origin && this.Destination == pair.Destination;
    }

    public bool Equals(ODPair pair)
    {
        if (pair == null)
        {
            return false;
        }

        return this.Origin == pair.Origin && this.Destination == pair.Destination;
    }

    public override int GetHashCode()
    {
        return this.Origin.GetHashCode() ^ this.Destination.GetHashCode();
    }

    #endregion
}

Equals

You have a bit of repetition between your Equals methods. Since no instance is equal to null, you can replace your entire overridden Object.Equals method with:

return Equals(obj as ODPair);

If the obj is not an ODPair, this will just pass in a null which will then return false.

At a pinch, XOR is a passable way of combining hash-codes. But it has collision issues:

• All instances where the origin and destination are the same will have the hash code of 0 (you mentioned this will never be the case for this class, but it's worth keeping in mind in general)

• Two instances where the origin and destination are swapped will have the same hash code.

• As Simon mentions in his answer, if the hashcodes being XORed occupy a small subset of the integers, there will be frequent collisions. This too is not likely to be much of a problem for you because the codes getting combined are from strings, so are likely to span the integers pretty well.

So of those it's likely to be the second, if any, which is of concern in this particular case.

My preference is to avoid the complexity of trying to implement my own hashing algorithm and let .NET do it for me:

return Tuple.Create(Origin, Destination).GetHashCode();

or:

return new {Origin, Destination}.GetHashCode();

Only if this presents a noticeable performance issue should you worry about trying to implement your own- premature optimization is still premature optimization even inside a GetHashCode method!

StackExchange Code Review Q#95660, answer score: 7