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.