24
namespace Dic
{
public class Key
{
    string name;
    public Key(string n) { name = n; }
}

class Program
{
    static string Test()
    {
        Key a = new Key("A");
        Key b = new Key("A");
        System.Collections.Generic.Dictionary<Key, int> d = new System.Collections.Generic.Dictionary<Key, int>();
        d.Add(a, 1);
        return d.ContainsKey(b).ToString();
    }

    static void Main(string[] args)
    {
        System.Console.WriteLine(Test());
    }
}
}

What should I change to get true?

Giorgi
  • 30,270
  • 13
  • 89
  • 125
SkyN
  • 1,417
  • 4
  • 17
  • 32

12 Answers12

47

You want true - but a and b are different objects.

You need to override GetHashCode and Equals on class Key

public class Key
{
    string name;
    public Key(string n) { name = n; }

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

    public override bool Equals(object obj)
    {
        Key other = obj as key;
        return other != null && other.name == this.name;
    }
}
Itay Karo
  • 17,924
  • 4
  • 40
  • 58
  • 7
    Alternatively, if `Key` is truly that simple, you could define it as a `struct`. – Toby Jun 10 '10 at 13:50
  • 7
    Why do I even bother answering these questions. Everyone beats me to it by several minutes :) – simendsjo Jun 10 '10 at 13:50
  • 1
    GetHashCode should return an `int` in the declaration ;) – Mark Seemann Jun 10 '10 at 13:58
  • @Toby and how would the struct thing help with this? Elaborate please – Ahmed Jun 10 '10 at 14:36
  • 1
    @Galilyou: I think Toby's referring to the fact that `ValueType.Equals` by default compares structs by checking each of their fields. It does so using reflection, however, so performance is compromised. Overriding `Equals` and `GetHashCode` is generally a better approach when the intention is to use a type as a key in a dictionary (in my opinion). – Dan Tao Jun 10 '10 at 15:08
10

It would probably help if you override Key.GetHashCode and Key.Equals.

In Key:

public override bool Equals(object obj)
{
    var k = obj as Key;
    if (k != null)
    {
        return this.name == k.name;
    }
    return base.Equals(obj);
}

public override int GetHashCode()
{
    return this.name.GetHashCode();
}
Mark Seemann
  • 225,310
  • 48
  • 427
  • 736
9

If you do not have the ability to override equality operators/Equals/GetHashCode as others have mentioned (as in, you do not control the source code of the object), you can provide an IEqualityComparer<Key> implementation in the constructor of the dictionary to perform your equality checks.

class KeyComparer : IEqualityComparer<Key>
{
    public bool Equals(Key x, Key y)
    {
        return x.Name == y.Name;
    }

    public int GetHashCode(Key obj)
    {
        return obj.Name.GetHashCode();
    }
}

As it stands, your Key is a reference object, so equality is only determined on reference unless you tell the world (or the dictionary) otherwise.

Anthony Pegram
  • 123,721
  • 27
  • 225
  • 246
  • 1
    Note: Of course, to have such an implementation, the members you wish to compare on must be externally accessible, they cannot be private. – Anthony Pegram Jun 10 '10 at 13:56
7

Overriding a class' GetHashCode and Equals methods so that it will work properly in a dictionary is not a very good approach. The way a dictionary behaves should be an implementation detail of the dictionary, not of whatever class is being used as a key. You'll get into trouble when you want to use the class in different dictionaries with different behavior. Or if you don't have access to the class source code.

A better mouse trap is to give the dictionary its own comparer. For example:

using System;
using System.Collections.Generic;

class Program {
    static void Main(string[] args) {
        var d = new Dictionary<Key, int>(new MyComparer());
        d.Add(new Key("A"), 1);
        Console.WriteLine(d.ContainsKey(new Key("a")));
        Console.ReadLine();
    }
    private class MyComparer : IEqualityComparer<Key> {
        public bool Equals(Key x, Key y) {
            return string.Compare(x.Name, y.Name, true) == 0;
        }
        public int GetHashCode(Key obj) {
            return obj.Name.ToUpper().GetHashCode();
        }
    }
    public class Key {
        public string Name { get; set; }
        public Key(string name) { Name = name; }
    }
}
Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
2

In order to use your own classes as dictionary keys, you should override GetHashCode and Equals. Otherwise it will use the memory address to check for equality.

    public class Key
    {
        string name;
        public Key(string n) { name = n; }

        public override int GetHashCode()
        {
            return name.GetHashCode();
        }

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

            return name == other.name;
        }
    }

simendsjo
  • 4,739
  • 2
  • 25
  • 53
1

you problem is that

new Key("A").Equals(new Key("A"))==false.

and

new Key("A").GetHashCode()!=new Key("A").GetHashCode()

fix that and it should work I think. To fix it override the Equals method and check if the name values are the same. You should also override GetHashCode if you are overriding Equals.

Sam Holder
  • 32,535
  • 13
  • 101
  • 181
  • 1
    To be precise, fixing `Equals` will *not* change the behaviour of `==` neither `!=`. These would have to be overridden as well (highly recommended!) but this isn’t required for `Dictionary`. – Konrad Rudolph Jun 10 '10 at 13:49
  • @Konrad, Microsoft [advises against](http://msdn.microsoft.com/en-us/library/ms173147%28VS.80%29.aspx) overriding == for mutable types like Key. – Matthew Flaschen Jun 10 '10 at 13:55
  • @Matthew: to be honest, I stopped heeding Microsoft’s coding guidelines quite some time ago when I noticed that they churned out lots of baseless, unjustified advice that contradicted my experience. In this particular case, it *can* actually be justified (but isn’t! Why?) but I still think the advice is backwards. It should rather be: make objects immutable if possible. If not, don’t override `Equals` either. And yes, that means such objects can’t be stored in Dictionaries. So what? Python has the same restriction with very similar semantics and it works well. – Konrad Rudolph Jun 10 '10 at 14:12
  • @Matthew: (cont’d) Making `Equals` and `operator ==` inconsistent violates [POLS](http://en.wikipedia.org/wiki/Principle_of_least_surprise) and doesn’t really serve a purpose. Had they supported operator overloading from the start and allowed virtual operators, this whole artificial dichotomy would never have arisen and `Equals` would never have existed. The only reason for its existence in the first place is that operators cannot be virtual in .NET. – Konrad Rudolph Jun 10 '10 at 14:17
  • I think this one has a quite simple reason. Programmers expect that the == result for a pair of objects won't change. For programmers familiar with C, Java, or non-overridden C#, this is the least surprising outcome. – Matthew Flaschen Jun 10 '10 at 14:17
  • 1
    @Matthew: I would contest that assumption. Programmers (beginners) are expecting side effects everywhere (immutable strings seem to be a major stumbling block, as evidenced by the questions about the return value of `String.Replace`). I also don’t see how this assumption would be different for `Equals`. – Konrad Rudolph Jun 10 '10 at 14:29
1

you need to override Equals and GetHashCode methods of your Key class.

Arseny
  • 7,251
  • 4
  • 37
  • 52
1

1. Override Equals, Get Hash Code, and the '==' Operator.

The Key class must override Equals in order for the Dictionary to detect if they are the same. The default implementation is going to check references only.

Here:

        public bool Equals(Key other)
        {
            return this == other;
        }

        public override bool Equals(object obj)
        {
            if (obj == null || !(obj is Key))
            {
                return false;
            }

            return this.Equals((Key)obj);
        }

        public static bool operator ==(Key k1, Key k2)
        {
            if (object.ReferenceEquals(k1, k2))
            {
                return true;
            }

            if ((object)k1 == null || (object)k2 == null)
            {
                return false;
            }

            return k1.name == k2.name;
        }

        public static bool operator !=(Key k1, Key k2)
        {
            if (object.ReferenceEquals(k1, k2))
            {
                return false;
            }

            if ((object)k1 == null || (object)k2 == null)
            {
                return true;
            }

            return k1.name != k2.name;
        }

        public override int GetHashCode()
        {
            return this.name == null ? 0 : this.name.GetHashCode();
        }

2. If Possible, use a struct.

You should use a struct for immutable datatypes like this, since they are passed by value. This would mean that you coulnd't accidentally munge two different values into the same key.

John Gietzen
  • 48,783
  • 32
  • 145
  • 190
  • This code isn’t safe (NRE in `GetHashCode` for one thing) and somewhat longer than it needs to be due to unnecessary code duplications. For a non-redundant implementation, check [my write-up on another question](http://stackoverflow.com/questions/104158/what-is-best-practice-for-comparing-two-instances-of-a-reference-type/104309#104309). – Konrad Rudolph Jun 10 '10 at 13:59
  • Alright, I fixed the errors, and I +1'd your implementation on the other question. – John Gietzen Jun 10 '10 at 15:05
0

they have the same values internally but a != b as they are 2 different variables.

AutomatedTester
  • 22,188
  • 7
  • 49
  • 62
0

You need to override the Equals and GetHashCode methods of your Key class. In your case you could compare based on the key's name (or on any other unique property if your class is more complex).

public class Key {
    string name;
    public Key(string n) { name = n; }

    public override bool Equals(object obj) {
        Key k = obj as Key;
        if (k == null)
            return false;
        return name.Equals(k.name);
    }

    public override int GetHashCode() {
        return name.GetHashCode();
    }
}
LorenzCK
  • 7,411
  • 2
  • 36
  • 28
0

Then you will need to override GetHashCode and Equals on the Key class.

Without doing that, you get the default implementation of both. Which results in the hashcode for a and b to be most likely not the same (I don't know how what the default implementation looks like), and a to be definitely not equal to b (the default Equals() implementation checks reference equality).

In your case, assuming "name" is not a null, it could be implemented as

   public class Key
   {
        string name;
        public override int GetHashCode()
        {
             return name.GetHashCode();
        }

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

            Key objAsKey = obj as Key;
            if (objAsKey == null)
            {
              return false;
            }

            return this.name.Equals(objAsKey.Name);
        }
    }

Whether this is a satisfactory hash is a different story, but it shows the principle, nevertheless.

Sam Holder
  • 32,535
  • 13
  • 101
  • 181
Willem van Rumpt
  • 6,490
  • 2
  • 32
  • 44
-1

ContainsKey in this case is comparing Key as objects and checking to see if the objects themselves are the same -- they are not. You need to implement IComparable or override Key.Equals or something along those lines to get it to do what you want.

Mitch
  • 659
  • 1
  • 7
  • 18
  • 1
    Careful – `Dictionary` is implemented in terms of a hash map so (unlike `SortedDictionary` and `SortedList`) implementing `IComparable` has *no* effect whatsoever. – Konrad Rudolph Jun 10 '10 at 13:55