Class implementation of IEquatable for use as a key in a dictionary

Question

I've got a class which consists of two strings and an enum. I'm trying to use instances of this class as keys in a dictionary. Unfortunately I don't seem to be implementing IEquatable properly. Here's how I've done it:

public enum CoinSide
{
    Heads,
    Tails
}

public class CoinDetails : IComparable, IEquatable<CoinDetails>
{
    private string denomination;
    private string design;
    private CoinSide side;

//...

    public int GetHashCode(CoinDetails obj)
    {
        return string.Concat(obj.Denomination, obj.Design, obj.Side.ToString()).GetHashCode();
    }

    public bool Equals(CoinDetails other)
    {
        return (this.Denomination == other.Denomination && this.Design == other.Design && this.Side == other.Side);
    }
}

However, I still can't seem to look up items in my dictionary. Additionally, the following tests fail:

    [TestMethod]
    public void CoinDetailsHashCode()
    {
        CoinDetails a = new CoinDetails("1POUND", "1997", CoinSide.Heads);
        CoinDetails b = new CoinDetails("1POUND", "1997", CoinSide.Heads);
        Assert.AreEqual(a.GetHashCode(), b.GetHashCode());
    }

    [TestMethod]
    public void CoinDetailsCompareForEquality()
    {
        CoinDetails a = new CoinDetails("1POUND", "1997", CoinSide.Heads);
        CoinDetails b = new CoinDetails("1POUND", "1997", CoinSide.Heads);
        Assert.AreEqual<CoinDetails>(a, b);
    }

Would someone be able to point out where I'm going wrong? I'm sure I'm missing something rather simple, but I'm not sure what.

You haven't overridden `bool Equals(object)` which is probably why the tests are failing. I'd expect the dictionary to be okay though. If you could show a short but *complete* program demonstrating the problem, that would be helpful. — Jon Skeet, Mar 04 '14 at 14:57

score 9 · Accepted Answer · edited May 23 '17 at 12:17

9

You class has to override Equals and GetHashCode:

public class CoinDetails 
{
    private string Denomination;
    private string Design;
    private CoinSide Side;

    public override bool Equals(object obj)
    {
        CoinDetails c2 = obj as CoinDetails;
        if (c2 == null)
            return false;
        return Denomination == c2.Denomination && Design == c2.Design;
    }

    public override int GetHashCode()
    {
        unchecked
        {
            int hash = 17;
            hash = hash * 23 + (Denomination ?? "").GetHashCode();
            hash = hash * 23 + (Design ?? "").GetHashCode();
            return hash;
        }
    }
}

Note that i've also improved your GetHashCode algorithm according to: What is the best algorithm for an overridden System.Object.GetHashCode?

You could also pass a custom IEqualityComparer<CoinDetail> to the dictionary:

public class CoinComparer : IEqualityComparer<CoinDetails>
{
    public bool Equals(CoinDetails x, CoinDetails y)
    {
        if (x == null || y == null) return false;
        if(object.ReferenceEquals(x, y)) return true;
        return x.Denomination == y.Denomination && x.Design == y.Design;
    }

    public int GetHashCode(CoinDetails obj)
    {
        unchecked
        {
            int hash = 17;
            hash = hash * 23 + (obj.Denomination ?? "").GetHashCode();
            hash = hash * 23 + (obj.Design ?? "").GetHashCode();
            return hash;
        }
    }                      
}

Now this works and does not require CoinDetails to override Equals+GetHashCode:

var dict = new Dictionary<CoinDetails, string>(new CoinComparer());
dict.Add(new CoinDetails("1POUND", "1997"), "");
dict.Add(new CoinDetails("1POUND", "1997"), ""); // FAIL!!!!

edited May 23 '17 at 12:17

Community

1
1

answered Mar 04 '14 at 15:00

Tim Schmelter

450,073
74
686
939

I removed the IEquatable reference and changed the methods to overrides, which allowed the program to run correctly. I'm now reading that "best hash code" post and will probably make changes accordingly. Thank you very much! – user2823789 Mar 04 '14 at 15:09
@user2823789: note that i've also edited my answer to provide another approach that does not require to modify the class itself. – Tim Schmelter Mar 04 '14 at 15:12
Yes, I noticed. However, I think I'll stick with overriding them. Also, interestingly, 23 is apparently a bad choice for one of the primes in the improved hashcode algorithm, as: "23 is no good choice, since(as of .net 3.5 SP1) Dictionary assumes good distribution modulo certain primes. And 23 is one of them. So if you have a dictionary with Capacity 23 only the last contribution to GetHashCode influences the compound hashcode. So I'd rather use 29 instead of 23." Nevertheless, you've been extremely helpful, so thank you. – user2823789 Mar 04 '14 at 15:18
@user2823789: _"If you've got a dictionary that small, it's unlikely to matter much."_ (J. Skeet) ;-) – Tim Schmelter Mar 04 '14 at 15:27
Can't believe I have to `override` `Equals` and (worse) `GetHashCode`!? Why couldn't the compiler auto-generate default implementations that recursively Reflect until they get down to Value Types and always use 17 as a base Hash Code and 23 (or 29) as a Hash Code increment? – Tom Nov 27 '18 at 22:39
@Tom how should the compiler know how you want to compare your objects? The default is that just references are compared which is efficient and correct. – Tim Schmelter Nov 28 '18 at 08:01
@Rango: a) I'm referring only to a "default" and only to `IEquatable` (not `IComparable`), so the order (e.g. precedence) of values being compared does not matter. If it did, I agree, that the compiler could not default an order (and even then, it could by: a) convention of declaration order (ala ORM Frameworks that *default* to the *convention* of the "ID" Column being the PK), or b) an `Attribute` to specify the order (again, ala ORM Frameworks that do so)). b) I said "recursively Reflect until they down to Value Types" which have perfectly valid default `Equals` Comparer. – Tom Nov 29 '18 at 16:25
@Rango: Also, I was only thinking of (the reason I found this thread) a `class` whose only purpose is to be the `TKey` Type of the `Dictionary`. Although, as I'd mentioned above re. `IComparable`, an Attribute could be used to specify which Properties are Key Properties (as well as the order of comparison). – Tom Nov 29 '18 at 17:02
@Rango: Imho, whether a Variable is a Value Type or a Reference Type for purposes of equality (or relative) comparison really should be an "implementation detail". *Ideally*, the default comparison *should* be of whether 2 Variables *represent* the same *value* (not what was used to *reference* the *value*), and the comparison of the *references* (not the *value*) should be the *exception* not vice versa. – Tom Nov 29 '18 at 17:17
@Rango: But seeing as how that ship has sailed (, burned and sunk), simply to avoid introducing breaking features, I would suggest that the compiler *should* simply at least provide a default, say 'ValueEquals` and `GetValueHashCode`, Methods that the `override` Methods could simply Call. – Tom Nov 29 '18 at 17:18
@Tom: such a default that makes assumptions which aren't true in most cases would break all code that already exists. – Tim Schmelter Nov 29 '18 at 17:26
@Rango: Btw, when I said "exception" above, I would still have the compiler provide a (say "RefEquals(object, object)") Method or Operator that could be used to compare the References when necessary, but the "default" comparison *should* be of the Values. – Tom Nov 29 '18 at 17:28
@Rango: Did you see my comment (8 mins) before your last one (about my *compromise* to "avoid introducing breaking features")? – Tom Nov 29 '18 at 17:30
@Tom: but there is no general `DefaultValueComparer` because all types are different and classes can contain members that are also classes or even `Dictionary>` and so on. People would use it because they don't want to learn how to implement a working `Equals`/`GetHashCode` themselves. And then they would come to stackoverflow and ask why it behaves so strange and is so slow. – Tim Schmelter Nov 29 '18 at 17:35
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/184486/discussion-between-rango-and-tom). – Tim Schmelter Nov 29 '18 at 17:42

Class implementation of IEquatable for use as a key in a dictionary

1 Answers1