C# Is this a bad way to implement GetHashCode?

Question

I'm trying to implement GetHashCode for an object that I have overridden Equals on.

public override bool Equals(object obj)
{
    var myobject = obj as MyObject;
    if (myobject == null)
        return false;

    if (myobject.SomeProperty == null || SomeProperty == null)
        return false;

    // All default SomeProperty's are equal
    if (myobject.SomeProperty.IsDefault)
        return SomeProperty.IsDefault;

    // Otherwise equality is based on ID
    return myobject.SomeProperty.ID == SomeProperty.ID;
}

public override int GetHashCode()
{
    if (SomeProperty != null && SomeProperty.IsDefault)
        return 0;
    else return base.GetHashCode();
}

Is this a reasonable way to do it, or is it likely to cause collisions with base.GetHashCode()?

EDIT: I appreciate the solutions given thus far, but the same question remains. If I do not implement a complete GetHashCode and rely on some other implementation, either base.GetHashCode(), or Guid.GetHashCode(), is there a chance for hash code collisions with the hardcoded 0 value? And if so, is there a simple way to avoid it?

What does the `Equals` look like? I'm thinking the answer is "no", but I'd need to see how `Equals` works to be sure. — Tim S., Oct 07 '13 at 19:54
Does the class extend `object`, or is `base.GetHashCode()` referring to another implementation? — Tim S., Oct 07 '13 at 19:55
The collisions are going to come from the fact that every single non-null `SomeProperty` value results in a hash of zero, so they all collide. That's *probably* a bad idea. Also, what's the type of `SomeProperty`? — Servy, Oct 07 '13 at 19:57
Here's some guidelines for GetHashCode: http://blogs.msdn.com/b/ericlippert/archive/2011/02/28/guidelines-and-rules-for-gethashcode.aspx. One thing to watch out for is if SomeProperty is mutable. If so, then you're violating the "Rule: the integer returned by GetHashCode must never change while the object is contained in a data structure that depends on the hash code remaining stable" rule. — Michael Kelley, Oct 07 '13 at 19:57
@hatchet I've never implemented GetHashCode before. I'm just looking for a simple way so that the hashcode of all objects whose IsDefault value is set to true is the same. For all other objects I just want it to use the standard hashcode logic. — ConditionRacer, Oct 07 '13 at 20:07
@TimS. Added Equals implementation. The class extends object. — ConditionRacer, Oct 07 '13 at 20:12
@Justin984 What's the type of `SomeProperty.ID`? Is it `int`? — Servy, Oct 07 '13 at 20:13
There is a very good chance you shouldn't be overriding Equality at all. Are `ID` and `SomeProperty` readonly or mutable? — H H, Oct 07 '13 at 20:41
@HenkHolterman Assuming the object is immutable then it would just mean that he should use an `IEqualityComparer` instead, and if he did, he'd still have the same problem in writing a definition of such a comparer. — Servy, Oct 07 '13 at 20:44

Servy · Answer 1 · 2013-10-07T20:23:02.173

Based on your Equals method, what you consider to truly be the "identity" of an object is the value of SomeProperty.ID, with some additional handling of null values/properties/etc.

That should be reflected in the hash code as well.

You should start out handing all of those edge cases, as you do in your Equals method. It seems like you kind sorta went down this path, but didn't quite get there. If the object's SomeProperty is null OR it IsDefault then it doesn't have an ID, and to us, all such objects are "the same" and should have the same hash code.

After that though, rather than leveraging the base class' hash code, you need to actually base the hash on the ID property, which is what your Equals method does next. Since ID is an Guid we know it has a sensible GetHashCode implementation, so we can just leverage that, and then you're done:

public override int GetHashCode()
{
    if (SomeProperty == null || SomeProperty.IsDefault)
        return 0;
    else 
        return SomeProperty.ID.GetHashCode();
}

Or, if it's easier for you to read, we could reverse the logic. Rather than saying, "If this doesn't have an ID, return zero, else return the ID's hash code" we can say, "If we have an ID, return it's hash code, else just return zero:

public override int GetHashCode()
{
    if (SomeProperty != null && !SomeProperty.IsDefault)
        return SomeProperty.ID.GetHashCode();
    else 
        return 0;
}

personally I think the first has a bit more symmetry with your Equals method, but the second seems a bit closer to what you were trying to do, which is why I'm throwing it up there.

Well that's easy. Thanks :) The same question remains though, how likely is it that SomeProperty.ID.GetHashCode() returns 0? — ConditionRacer, Oct 07 '13 at 20:21
If there is a large percentage of the population without SomeProperty then won't this not be well distributed? Why not use return base.GetHashCode() as in my solution http://stackoverflow.com/a/19233760/2145211 for the else — Harrison, Oct 07 '13 at 20:28
@Harrison Because, as I said in the answer, this assumes that, by definition, all of the objects that don't have an ID value are *the same*. Not that all objects that don't have an ID value instead use their reference to determine equality. If you have 100 different objects, all with no `ID` value, and you put them all into a `HashSet` would you want them to all be different distinct items? If that is required functionality, then yes, you do that. If, instead, you want them to be considered equal, they need to have the same hash. — Servy, Oct 07 '13 at 20:38

score 1 · Answer 2 · edited May 23 '17 at 12:05

That is not a good solution. The documentation shows

A hash function must have the following properties:

If two objects compare as equal, the GetHashCode method for each object must return the same value. However, if two objects do not compare as equal, the GetHashCode methods for the two object do not have to return different values.

The GetHashCode method for an object must consistently return the same hash code as long as there is no modification to the object state that determines the return value of the object's Equals method. Note that this is true only for the current execution of an application, and that a different hash code can be returned if the application is run again.

For the best performance, a hash function must generate a random distribution for all input.

You are presumably returning 0 for many unequal objects and although this does not break the tenant "if two objects do not compare as equal, the GetHashCode methods for the two object do not have to return different values" if too many objects return 0 then you will not fulfill "For the best performance, a hash function must generate a random distribution for all input."

In addition,

Derived classes that override GetHashCode must also override Equals to guarantee that two objects considered equal have the same hash code; otherwise, the Hashtable type might not work correctly.

Therefore, you're implementation of Equals should also be posted to make sure that the GetHashCode is valid.

I would suggest updating it to

public override int GetHashCode()
{
    if (SomeProperty == null || SomeProperty.IsDefault)
        return base.GetHashCode() ;
    else return SomeProperty.ID.GetHashCode();
}

Also, for a good discussion of the GetHashCode implemention you could look at this post

In addition in your Equals if the SomeProperty==null in both objects you are returning false, is that intentional?

or did you mean

if (myobject.SomeProperty == null && SomeProperty == null)
    return true;
if (myobject.SomeProperty == null || SomeProperty == null)
    return false;

C# Is this a bad way to implement GetHashCode?

2 Answers2