Have I implemented Equals()/GetHashCode() correctly?

Question

The program was working with this implementation:

class Instrument
{
    public string ClassCode { get; set; }
    public string Ticker { get; set; }
    public override string ToString()
    {
        return " ClassCode: " + ClassCode + " Ticker: " + Ticker + '.';
    }
}

But because I need to use Instrument in Dictionary I've decided to implement equals/hashcode:

class Instrument
{
    public string ClassCode { get; set; }
    public string Ticker { get; set; }
    public override string ToString()
    {
        return " ClassCode: " + ClassCode + " Ticker: " + Ticker + '.';
    }

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

        Instrument instrument = obj as Instrument;
        if (instrument == null)
            return false;

        return ((ClassCode.Equals(instrument.ClassCode)) && (Ticker.Equals(instrument.Ticker));
    }

    public override int GetHashCode()
    {
        int hash = 13;
        hash = (hash * 7) + ClassCode.GetHashCode();
        hash = (hash * 7) + Ticker.GetHashCode();
        return hash;
    }
}

Now the program has stopped working. In such or similar places I receive "KeyNotFoundException":

if (cache.Keys.Any(instrument => instrument.Ticker == newTicker && instrument.ClassCode == newClassCode))

Is it possible that some pieces of the code assume that equals and hashcode IS NOT implemented? Or probably I just implemented them wrong? Sorry I'm not familiar with such advanced features in C# as the last piece of code and don't know how it is connected with equals or hashCode.

Negligible

Have a look at what Eric Lippert has to say on the matter - blogs.msdn.com/b/ericlippert/archive/2011/02/28/… In short your hash code should remain constant even if the instance mutates, so any implementation you do, should have this as the primary objective. – Izzy

At the moment in time I'm on the go so I'll only throw in a link I found useful for the equals part... geekswithblogs.net/akraus1/archive/2010/02/28/138234.aspx – Geminate

Answer 1

Your HashCode and Equals methods should depend only on immutable properties - your implementation uses ClassCode and Ticker which both have setters and are therefore mutable.

Aggrandize

Thank you Joe, I didn't read why c# has so strange restrictions so far. I will read suggested articles tonight. However so far it sounds very very strange. If some object was equal to another, and then some property of one object was changed, I expect that objects not equals anymore! So "equals" must include all properties and especially mutable properties.... It's the way I do things in Java :) – Negligible

@HuBeZa: imagine you put object into Dictionary, it will be assigned to bucket basing on its computed hash, say it goes to bucket #1. Now, you modify Ticker and/or ClassCode in object you've just put in and perform lookup with .Contains. Dictionary then calls GetHashCode to determine which bucket it should search for our object - and thanks to modified values, it assumes it's #2 instead of #1. It checks bucket #2 - your object is not there. Contains returns with false, on object you just put in dictionary. This is why you want immutable members for hash generation. – Levona

@jimmy_keen, excellent point. So I'm guessing that dictionary doesn't receive OnPropertyChanged notification for its keys, or something of that matter. So, the key hash is computed only when added. I wonder why I never saw an article about that. – Orthodontia

@jimmy_keen I consider that as a valuable feature, not a danger :) – Gliwice

Answer 2

First, instead of using cache.Keys.Any you can just use ContainsKey.

bool contains = cache.ContainsKey(
    new Instrument { Ticker = newTicker, ClassCode = newClassCode });

The first iterate over the whole keys list - O(n), while the second uses Dictionary's built in hash table implementation - O(1).

Second, check for null reference in your implementation:

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

    Instrument instrument = obj as Instrument;
    if (instrument == null)
        return false;

    // 1. string.Equals can handle null references.
    // 2. object.ReferenceEquals for better preformances when it's the same object
    return (object.ReferenceEquals(this, instrument)) ||
        (string.Equals(ClassCode, instrument.ClassCode) &&
        string.Equals(Ticker, instrument.Ticker));
}

public override int GetHashCode()
{
    int hash = 13;
    if (ClassCode != null)
        hash = (hash * 7) + ClassCode.GetHashCode();
    if (Ticker!= null)
        hash = (hash * 7) + Ticker.GetHashCode();

    return hash;
}

Other than that, I can't see a problem.

Orthodontia

regarding "null" references, it's by design that my Instrument object can not contain "null" strings, so I would actually prefer to have NPE raised if some field is null accidentely. I think that codes uses cache.Keys.Any because equals and hashCode was not implemented (then ContainsKey will not work I suppose). The question is how is it possible to broke cache.Keys.Any implementing Equals/HashCode... – Negligible

Answer 3

But because I need to use Instrument in Dictionary I've decided to implement equals/hashcode

That's the wrong reason. Your class already has implementations for Equality and GetHashCode that are suitable, efficient and tested for use in a Dictionary.

Have I implemented Equals()/GetHashCode() correctly?

No. You are missing an overload for == to start with. And it will only be reliable when you make Instrument immutable.

Your best course of action is not to override any of those members.

Also see this MSDN advice. Notice the "guarantees of Equals" and

Overriding operator == in non-immutable types is not recommended.

Barbabas