How can I identify a bad implementation of GetHashCode?

Question

I have an implementation of GetHashCode which I believe to be fairly robust, but, honestly, I dredged it up from the depths of the internet and, while I understand what is written, I don't feel qualified to describe it as a 'good' or a 'bad' implementation of GetHashCode.

I've done a lot of reading on StackOverflow about GetHashCode. Is there a sample why Equals/GetHashCode should be overwritten in NHibernate? I think this thread is probably the best source of information, but it still left me wondering.

Consider the following entity and its given implementation of Equals and GetHashCode:

public class Playlist : IAbstractDomainEntity
{
    public Guid Id { get; set; }
    public string Title { get; set; 
    public Stream Stream { get; set; }
    //  Use interfaces so NHibernate can inject with its own collection implementation.
    public IList<PlaylistItem> Items { get; set; }
    public PlaylistItem FirstItem { get; set; }
    public Playlist NextPlaylist { get; set; }
    public Playlist PreviousPlaylist { get; set; }

    private int? _oldHashCode;
    public override int GetHashCode()
    {
        // Once we have a hash code we'll never change it
        if (_oldHashCode.HasValue)
            return _oldHashCode.Value;

        bool thisIsTransient = Equals(Id, Guid.Empty);

        // When this instance is transient, we use the base GetHashCode()
        // and remember it, so an instance can NEVER change its hash code.
        if (thisIsTransient)
        {
            _oldHashCode = base.GetHashCode();
            return _oldHashCode.Value;
        }
        return Id.GetHashCode();
    }

    public override bool Equals(object obj)
    {
        Playlist other = obj as Playlist;
        if (other == null)
            return false;

        // handle the case of comparing two NEW objects
        bool otherIsTransient = Equals(other.Id, Guid.Empty);
        bool thisIsTransient = Equals(Id, Guid.Empty);
        if (otherIsTransient && thisIsTransient)
            return ReferenceEquals(other, this);

        return other.Id.Equals(Id);
    }
}

The amount of safety-checking touted in this implementation seems over the top. It inspires confidence in me -- assuming that the person who wrote this understood more corner cases than I -- but also makes me wonder why I see so many simplistic implementations.

Why is it important to override GetHashCode when Equals method is overridden? Look at all of these different implementations. Below is a simple, yet highly rated, implementation:

  public override int GetHashCode()
  {
        return string.Format("{0}_{1}_{2}", prop1, prop2, prop3).GetHashCode();
  }

Would this implementation be better or worse than the one I've provided? Why?

Are both equally valid? Is there a standard 'guideline' which should be followed when implementing GetHashCode? Are there any obvious flaws with the above implementation? How can one create test cases to validate ones implementation of GetHashCode?

Answer 1

GetHashCode should match concept of "equal" for your classes/environment (in addition to be constant while in a container and fast).

In normal cases "equal" is comparing all fields of corresponding objects (value type comparison). In this case simple implementation that somehow merges hash codes of all fields will suffice.

My understanding that in NHibernate's case "equal" is significantly more tricky and as result you see complicated implementation.I believe it is mainly due to the fact that some object properties may not be yet available - in such case comparing "identity" of object is enough.

Answer 2

It's unfortunate that while there two different questions that an equality-test method could meaningfully ask of any pair of object references X and Y, there's only one Equals method and one GetHashCode method.

• Assuming X and Y are of the same type(*), will all members of X always behave the same as corresponding methods of Y? Two references to different arrays would be reported as unequal under this definition even if they contain matching elements, since even if their elements are the same at one moment in time, that may not always be true.

• Assuming X and Y are of the same type(*), would simultaneously replacing all references to object X with references to object Y, and vice versa, affect any members of either other than an identity-based GetHashCode function? References to two distinct arrays whose elements match would be reported as equal under this definition.

(*) In general, objects of different types should report unequal. There might be some cases where one could argue that objects of different private classes which are inherited from the same public class should be considered equal, if all of the code which has access to the private classes only stores reference in the matching public type, but that would be at most a pretty narrow exception.

Some situations require asking the first question, and some require asking the second; the default Object implementation of Equals and GetHashCode answer the first, while the default ValueType implementations answer the second. Unfortunately, the selection of which comparison method is appropriate for a given reference is a function of how the reference is used, rather than a function of the referred-to instance's type. If two objects hold references to collections which they will neither mutate nor expose to code that might do so, for the intention of encapsulating the contents thereof, equality of the objects holding those references should depend upon the contents of the collections, rather than their identity.

It looks as though code is sometimes using instances of type PlayList in ways where the first question is more appropriate, and sometimes in ways where the second would be more appropriate. While that may be workable, I think it might be better to have a common data-holder object which can be wrapped if necessary by an object whose equality-check method would be suitable for one use or the other (e.g. have a PlaylistData object which can be wrapped by either a MutablePlaylist or an ImmutablePlaylist). The wrapper classes could have InvalidateAndMakeImmutable or InvalidateAndMakeMutable methods which would invalidate the wrapper and return a new wrapper around the object (using wrappers would ensure that the system would know whether a given Playlist reference could be exposed to code that might mutate it).