2

I have already read tons of articles on properly implementing Equals() and GetHashCode(). I have some classes with lots of member variables for which I had to implement an Equals() method. So far, so good. However, when coming to implementing GetHashCode() I always get stuck finding a good implementation. As I know for certain classes that they will never be used in a dictionary or hash table, I didn't want to invest time in writing a good GetHashCode() implementation so I came up with the following dummy "default" implementation:

public override int GetHashCode()
{
    Debug.Fail( "If you see this message, implement GetHashCode() properly" );

    // Always return the same value for all instances of this class. This ensures
    // that two instances which are equal (in terms of their Equals() method)
    // have the same hash code.
    return 0;
}

I wanted to ask for your opinions if this is a feasible approach.

On one hand, I don't want to spend time on implementing GetHashCode() when I don't have to. On the other hand, I want my code to be clean and valid. The above implementation ensures that two instances which have the same Equals() result also have the same hash code. Of course if it really would be used as a key in a dictionary, the performance would be bad. But for this case you would get the debug assertion.

Are there any drawbacks in the above approach which I might have missed?

Tobias
  • 2,089
  • 2
  • 26
  • 51
  • Take a look at this SO question. I wouldn't say it's a duplicate, but it's definitely related: http://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode – Konamiman Sep 03 '14 at 14:39
  • 1
    Personal opinion: significant differences (like crash vs. success in your sample) between debug and release build are bad idea. – Alexei Levenkov Sep 03 '14 at 14:39
  • @AlexeiLevenkov There is no significant difference in this code between debug and release. It makes no difference if the `Debug.Fail` is executed and ignored or not. See also my comment to Jon Skeet's answer below. – Tobias Sep 03 '14 at 15:41
  • @Tobias abort/ignore dialog (default implementation of Debug.Fail for console/win form) indeed can be considered as "no-op"... That partially why I've said "personal opinion". – Alexei Levenkov Sep 03 '14 at 15:50

1 Answers1

4

If you really don't want to implement it, I would recommend throwing an exception instead (e.g. NotImplementedException) - then you're not just relying on Debug.Fail working. If you end up in this situation in a production environment, then failing quickly may well be the best option.

But to be honest, once you've implemented equality it's usually really easy to write GetHashCode (with code like this, perhaps) - and if it's not easy then that's often a sign that your Equals method will violate its contract anyway.

Note that GetHashCode isn't just used for dictionaries - if you try to join two lists in LINQ based on a key of your type, that will use GetHashCode too. Basically, not implementing it violates the principle of least astonishment. Unless you're the only person using your code (and you have a perfect memory or are willing to check each time you use the type), you should implement it properly, IMO.

Community
  • 1
  • 1
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • The above implementation is not relying on `Debug.Fail`. It's just a hint to a developer that there is something to optimize. The implementation/the result of `GetHashCode()` is 0. This works always, although it has not a good performance if really used as a key. I know that in LINQ queries `GetHashCode()` might be called, but currently it is not. And if it will be in the future, a developer will get the assertion and if he ignores it, the code will work nevertheless. – Tobias Sep 03 '14 at 15:39
  • BTW, throwing an exception would violate the rule that you should never throw an exception in `GetHashCode()`. I used to do that in former times, but the above approach doesn't need it. – Tobias Sep 03 '14 at 15:43
  • @Tobias: GetHashCode *will* be called if you use Join, Distinct, GroupBy etc. If an assumption ("this method won't be called") is violated, then I'd much rather find out about it immediately than have it work in testing due to not noticing, then cripple production usages. – Jon Skeet Sep 03 '14 at 15:55
  • If my class didn't implement `Equals()`, it could not be used for a couple of thing as well. If I as a programmer use an existing class and call `Equals()` or some LINQ query which needs `Equals()` then I first have to look into the class'es implementation to see if it has it implemented. Otherwise my code probably wouldn't work. The same I would do with `GetHashCode()` - if I used the class as a key, I would look for an implementation. This above implementation does always work, even when the developer does not look at it first. So is your point the bad performance? – Tobias Sep 04 '14 at 07:02
  • @Tobias: No, if something doesn't override `Equals` or `GetHashCode`, you'll just get reference identity. Yes, you need to be aware of that. My point is bad performance which may well not be obvious until it's a real problem in production - whereas throwing an exception is obvious even in very small test cases. But overriding `GetHashCode` instead is usually simple and gives you the best of all worlds. – Jon Skeet Sep 04 '14 at 07:50
  • If an application uses `Distinct` on what will always be small lists of items, having the dictionary fall back to linear search on a single bucket chain should be just fine. If it's used on lists whose size will vary with overall data set size, then steady linear degradation in per-item performance with large data sets should be noticeable during testing with data sets that are anywhere close to those used in production (unlike e.g. the performance of the VB6 collection which seems to have worse-than-cubic degradation beyond a certain size). – supercat Feb 21 '15 at 22:02