Instances of my class don't want to act properly as dictionary keys interchangeably with strings

Question

I created a sort of string wrapper class and want to use its instances as dictionary keys interchangeably with usual strings. I overrode GetHashCode and Equals and got results that seem strange. I've isolated the issue. Please look at my code and explain me why does second lookup returns null.

void Main()
{
    var foo = new StringWrapper("foo");
    var h = new Hashtable {{ foo, "bar" }};
    Console.WriteLine(h["foo"]);
    Console.WriteLine(h[foo]); // null ??
}

public class StringWrapper
{
    readonly string wrapped;
    public StringWrapper(string s) {
        wrapped = s;
    }
    public override bool Equals(object obj) {
        return wrapped.Equals(obj);
    }
    public override int GetHashCode() {
        return wrapped.GetHashCode();
    }
    public override string ToString() {
        return wrapped;
    }
}

What value is that wrapper adding that you can't get from the out of the box String? — duffymo, Aug 21 '12 at 17:46
@duffymo it's just an isolated issue for people to help me find the bug — thorn0, Aug 21 '12 at 17:54
I think the whole idea of "string wrapper class and want to use its instances as dictionary keys interchangeably with usual strings" is wrong headed. I'd love to hear why you think this is necessary. Why fix a bug when the reasons for writing the class in the first place are dubious? — duffymo, Aug 21 '12 at 18:01
@duffymo It's not necessary. In my specific situation, it's just more beautiful and readable. — thorn0, Aug 21 '12 at 18:32

score 6 · Answer 1 · answered Aug 21 '12 at 17:38

6

Keep in mind that in Equals(object obj), obj won't be a string, it will be a StringWrapper - and a string will never say that it is equal to something that is not a string. You need to extract the wrapped string from obj. Keep in mind that you should check for null and take into account that if someone uses StringWrapper for something else than putting it into a hash table, obj could also be any other type.

answered Aug 21 '12 at 17:38

Aasmund Eldhuset

37,289
4
68
81

Thank you. It turned out to be so easy. – thorn0 Aug 21 '12 at 18:21

score 3 · Accepted Answer · answered Aug 21 '12 at 17:41

3

Your equality implementation isn't symmetric, nor reflexive:

StringWrapper wrapper = new StringWrapper("foo");

Console.WriteLine(wrapper.Equals(wrapper)); // False
Console.WriteLine(wrapper.Equals("foo")); // True
Console.WriteLine("foo".Equals(wrapper)); // False

So you're violating the rules specified in the docs for Object.Equals. Hashtable (and other classes) expect you not to violate those rules, and won't work properly when you violate them.

It sounds like you need a custom collection, rather than a wrapper which tries to pretend that the wrapper is equal to the original value.

(As an aside, why are you still using non-generic Hashtable rather than Dictionary<,>?)

answered Aug 21 '12 at 17:41

Jon Skeet

1,421,763
867
9,128
9,194

What a strange feeling. Your book (translated to Russian) is lying on the table behind me. And you write me here. It feels almost like a talking book! :) No, I don't need a custom collection. What I need is a sort of smart string constants with some additional abilities that still can be used as usual strings. It's needed for a legacy project started a long time ago, which is, yeah, full of Hashtables. So don't be surprised. For another example, in the same project, we use operator overloading to compose DB queries (==, !=, >, <, etc.). Just because LINQ didn't exist at that time. – thorn0 Aug 21 '12 at 18:11
1

@thorn: If you need a collection which doesn't mind when you violate the normal rules of equality, then you *do* need a different collection. It's not clear how the accepted answer will help you, as it's still not going to let you put a string in and look up via a wrapper or vice versa, without relying on undocumented behaviour. – Jon Skeet Aug 21 '12 at 18:33
Yeah, you're right. It doesn't work if I use a string as a key to add a value and then try to look up by a wrapper. :( – thorn0 Aug 21 '12 at 18:42
1

@thorn: Which direction it works on is implementation-specific; it will depend on whether the collection calls `foo.Equals(bar)` or `bar.Equals(foo)`. – Jon Skeet Aug 21 '12 at 18:48

score 1 · Answer 3 · answered Aug 21 '12 at 17:39

The problem is your Equals method:

public override bool Equals(object obj) {
    // wrapped is a String, but obj is (usually) a StringWrapper
    return wrapped.Equals(obj);
}

You need something like this:

public override bool Equals(object obj) {
    var other = obj as StringWrapper;
    return other != null && wrapped.Equals(other.wrapped);
}

EDIT: As per Jon's answer, you also might want to be able to compare to Strings:

public override bool Equals(object obj) {
    var other = obj as StringWrapper;
    return (other != null && wrapped.Equals(other.wrapped))
           || Object.Equals(wrapped, (obj as String));
}

Instances of my class don't want to act properly as dictionary keys interchangeably with strings

3 Answers3