What should GetHashCode return when object's identifier is null?

Question

Which of the following is correct/better, considering that identity property could be null.

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

OR

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

Update 1: Updated 2nd option.

Update 2: Below are the Equals implementations:

public bool Equals(IContract other)
{
    if (other == null)
        return false;
    if (this.ID.Equals(other.ID)) {
        return true;
    }
    return false;
}

public override bool Equals(object obj)
{
    if (obj == null)
        return base.Equals(obj);
    if (!obj is IContract) {
        throw new InvalidCastException("The 'obj' argument is not an IContract object.");
    } else {
        return Equals((IContract)obj);
    }
}

And ID is of string type.

Undervalue

Answer 1

It really depends on what you want equality to mean - the important thing is that two equal objects return the same hashcode. What does equality mean when ID is null? Currently your Equals method would have to return true if the ID properties have the same value... but we don't know what it does if ID is null.

If you actually want the behaviour of the first version, I'd personally use:

return ID == null ? base.GetHashCode() : ID.GetHashCode();

EDIT: Based on your Equals method, it looks like you could make your GetHashCode method:

return ID == null ? 0 : ID.GetHashCode();

Note that your Equals(IContract other) method could also look like this:

return other != null && object.Equals(this.ID, other.ID);

Your current implementation will actually throw an exception if this.ID is null...

Additionally, your Equals(object) method is incorrect - you shouldn't throw an exception if you're passed an inappropriate object type, you should just return false... ditto if obj is null. So you can actually just use:

public override bool Equals(object obj)
{
    return Equals(obj as IContract);
}

I'm concerned about equality based on an interface, however. Normally two classes of different types shouldn't be considered to be equal even if the implement the same interfaces.

Extravehicular

equality for interfaces should be put into its own class implementing IEqualityComparer<T> that will then be used to instantiate e.g. a dictionary by calling Dictionary<TKey, TValue>(IEqualityComparer<TKey>) – Marco

@Oliver: Agreed, if that's really necessary... but I can't remember the last time I wanted to compare objects of two different types with a common interface. – Extravehicular

@Jon: Thanks for your reply, its always a C# lesson. :) The reason behind employing equality based on interface is that my app is designed in such a way that interfaces have a one-to-one mapping with the classes (entities) and additionally, within the entire app, objects of type Contract are being used as IContract so I'd have to cast it to Contract at each place in the other case to check for equality. – Undervalue

@Dienekes: Well, your Equals(object) method could take care of that. If you mean it to only compare as equal with other Contract classes, make that the case. If you really want equality for IContract, I would suggest creating an IEqualityComparer<IContract> instead. – Extravehicular

Answer 2

You can simply return 0; , you need to return same HashCode for same values and 0 wont be often returned by ID.GetHashCode() so such Hash function can be pretty ok for any needs. Since your not combining any values (like ID and Name Hashes ) its pretty clear ID is the defining source of HashCode so fixed 0 for Null ID sounds reasonable.

Otherwise it might be true that your whole approach on GetHashCode override only taking into account ID field is wrong ( and you need to combine several fields to compute hash from them)

After your edits I can say that second Equals override has too much code , simply replace it with

public override bool Equals(object obj)
{
    return Equals(obj as Contract);
}

Your Equals(IContract contract) override appears buggy to me cause only thing defining contract is ID and if IContract has more fields than ID its going to be a bad Equals override.

PS: Actually if IContract is an interface you probably need to replace your IEquatable<IContract> to a concrete IEquatable<ClassName> contract because its going to be bad design to be able to return that Different class intstances implementing the same interface are equal cause equality by definition requires to check that objects have the same Type on the first stage of equality check (usually in like 99,9% cases)

Maelstrom

^^ thanks. btw, my app is designed in such a way that the interfaces have a one-to-one mapping with the classes (entities), that's why I inherited the IEquatable in the interface rather class. Additionally, within the entire app, objects of type Contract are being used as IContract so I'd have to cast it to Contract at each place in the other case to check for equality. – Undervalue

Answer 3

Perhaps what you want is something like this?

override int GetHashCode()
{
    if (ID != null)
        return ID.GetHashCode();

    return DBNull.Value.GetHashCode();
}

The important thing is this, should two objects with null IDs be considered equal?

Rodina