Search code examples
c#.netiequalitycomparer

Should I raise exceptions in Equals() and GetHashCode() if true comparison needs an IEqualityComparer?


One of my classes has a horrible requirement that resolving one of it's fields requires a service to be brought in by Dependency Injection, which is obviously not possible in a model in the standard Equals() and GetHashCode() calls. (Yes, I'd prefer it not to, bad practice etc, but I'm kind of stuck with it as a business requirement, unfortunately)

I can solve this by creating a Comparer class using IEqualityComparer<T>, but this leaves me with the default Object.Equals() and GetHashCode() being implemented, which may give misleading results when called.

As the presence of the IEqualityComparer is kind of 'hidden' unless you know about it, is it reasonable practice to override the Equals() and GetHashCode to return an exception to say that comparisons should use the Comparer? (Maybe just an Assert so that it only dies in debug/tests)


Solution

  • Throwing an exception like NotSupportedException is better than giving an incorrect answer, although since this is a class, arguably reference equality would suffice as the default, just using the external equality comparer for the custom functionality. But if that is going to cause confusion (in particular with people accidentally using the default API when they should be using the custom one); I wouldn't hesitate. The main problem you'll see is things like Contains checks blowing up, since classes aren't often used as dictionary keys.

    As for only doing this in DEBUG builds... well, if it is wrong: it is wrong. If there's a scenario you aren't currently testing but that is used in prod, IMO it is better to become aware of that fact than to not. Although perhaps you might use an environment variable it similar to disable it in case you can't conveniently deploy a fixed build at short notice.