Overriding Object.Equals() instance method in C#; now Code Analysis / FxCop warning CA2218: "should also redefine GetHashCode". Should I suppress this in my case?

+4 A:

My $0.10 worth? Implement GetHashCode.

As much as you say you'll never, ever need it, you may change your mind, or someone else may have other ideas on how to use the code. A working GetHashCode isn't hard to make, and guarantees that there won't be any problems in the future.

Steven Sudit 2010-03-25 14:47:50

If I document my class with *"don't use this as a key in a collection"*, would it be fair game for me to throw a *NotImplementedException* for my class's GetHashCode()? What are the problems with that approach?

Chris W. Rea 2010-03-25 14:59:48

You could, but what's the real benefit? A class with a special limitation that throws exceptions when tossed into a Dictionary is a liability. Implementing GetHashCode makes it fit right in with our reasonable expectations.

Steven Sudit 2010-03-25 15:15:46

+5 A:

As soon as you forget, or another developer who isn't aware uses this, someone is going to have a painful bug to track down. I'd recommend simply implementing GetHashCode correctly and then you won't have to worry about it. Or just don't use Equals for your special equality comparison case.

ermau 2010-03-25 14:47:57

But, what about the idea in my last paragraph: implementing GetHashCode() to throw a NotImplementedException? It would be more difficult for a developer to mistakingly use it as a key - it would blow up (by design).

Chris W. Rea 2010-03-25 14:54:20

While it may alleviate that mistake, it would be better to define the semantics now rather than later.

ermau 2010-03-25 15:04:31

+4 A:

You should not suppress it. Look at how your equals method is implemented. I'm sure it compares one or more members on the class to determine equality. One of these members is oftentimes enough to distinguish one object from another, and therefore you could implement GetHashCode by returning membername.GetHashCode();.

klausbyskov 2010-03-25 14:48:29

It actually compares almost all of the members of the class to determine equality. The class contains assumptions for a series of calculations. Any one of them being different is sufficient for me to consider the objects different. Some of the assumptions are arrays or collections of other numbers. So GetHashCode would be necessarily as complex if it were implemented (correctly).

Chris W. Rea 2010-03-25 14:50:55

@Chris: It doesn't have to be. A simple but often acceptable implementation is to XOR the GetHashCode values of the members.

Steven Sudit 2010-03-25 15:16:59

An even simpler and acceptable (but not usually optimal) implementation is to return the GetHashCode() value of only one field. That's what happens for a struct.

Hans Passant 2010-03-25 15:30:10

@klasbyskov, @Steven Sudit, @nobugz: Yes, I just realized my naivety: GetHashCode() doesn't *necessarily* need to factor in *all* of the members referred to by Equals() -- just enough of them to be "efficient" as a hash code. This was made obvious to me when Eric Lippert mentioned the simplest/worst case: *"if you don't want to be that harsh: Override GetHashCode; make it always return zero."

Chris W. Rea 2010-03-25 16:01:38

I'm accepting this as the answer. The assist goes to Eric Lippert.

Chris W. Rea 2010-03-25 16:04:41

+4 A:

The GetHashCode and Equals methods work together to provide value-based equality semantics for your type - you ought to implement them together.

For more information on this topic please see these articles:

Shameless plug: These articles were written by me.

Andrew Hare 2010-03-25 14:52:53

Does GetHashCode() ever get called by the framework, excluding when an object is used as a key in a collection?

Chris W. Rea 2010-03-25 14:57:01

I am not sure but the important thing to remember is that at any point in the future it _could_ be called and since the current recommendation is that you implement the method it would be better to do so.

Andrew Hare 2010-03-25 15:03:41

+7 A:

If you are reallio-trulio absosmurfly positive that you'll never use the thing as a key to a hash table then your proposal is reasonable. Override GetHashCode; make it throw an exception.

Note that hash tables hide in unlikely places. Plenty of LINQ sequence operators use hash table implementations internally to speed things up. By rejecting the implementation of GetHashCode you are also rejecting being able to use your type in a variety of LINQ queries. I like to build algorithms that use memoization for speed increases; memoizers usually use hash tables. You are therefore also rejecting ability to memoize method calls that take your type as a parameter.

Alternatively, if you don't want to be that harsh: Override GetHashCode; make it always return zero. That meets the semantic requirements of GetHashCode; that two equal objects always have the same hash code. If it is ever used as a key in a dictionary performance is going to be terrible, but you can deal with that problem when it arises, which you claim it never will.

All that said: come on. You've probably spent more time typing up the question than it would take to correctly implement it. Just do it.

Eric Lippert 2010-03-25 15:20:15

Good answer. And re: *"You've probably spent more time typing up the question than it would take to correctly implement it."* ... you're right *in this case* but I actually have a *series* of N objects that need something like Equals() to support my optimization. Trying to avoid N* the work. ;-)

Chris W. Rea 2010-03-25 15:49:51

And thanks especially for pointing out that LINQ may imply usage of GetHashCode(). That is particularly enlightening.

Chris W. Rea 2010-03-25 15:51:29

ansaurus

tags:

views:

answers:

Overriding Object.Equals() instance method in C#; now Code Analysis / FxCop warning CA2218: "should also redefine GetHashCode". Should I suppress this in my case?

related questions