0

I'm working on a simple application with a few classes. This all started when I wanted to use the Remove method on a List<Car>. This method requires that you override the Equals and the GetHashCode methods for the Car type. In this situation, I decided to implement an ID property on the Car class. That way, my Equals method simply checks for ID equality, and my GetHashCode method returns base.GetHashCode().

Is this a good approach, or is implementing a GUID for a small class too heavy-handed? There wouldn't be any need for it without the reasons I explained above. The only requirement for uniqueness for this Car type is that it be unique within the List<T> collection to which it belongs. But adding the GUID property seemed like the quickest way around the GetHashCode mess. BTW, there are no int properties on my Car type.

Justin
  • 84,773
  • 49
  • 224
  • 367
Andrew B Schultz
  • 1,512
  • 1
  • 23
  • 41
  • 2
    Just to clarify, the `Remove` method doesn't require the overrides, by default it uses [`EqualityComparer`](http://msdn.microsoft.com/en-us/library/cd666k3e.aspx). – Adam Houldsworth May 28 '12 at 15:54
  • That's helpful Adam, thanks; I was just using the default implementation, but it wasn't removing any of the objects in the list. So I started reading and found some materials that made it sound like you needed to override Equals(), which then required you to override GetHashCode(). – Andrew B Schultz May 28 '12 at 15:57

2 Answers2

3

There wouldn't be any need for it without the reasons I explained above.

If your class doesn't logically have an ID, then it certainly seems odd to include it just for the sake of equality.

For example, if you have two instances which have equal properties for everything apart from ID, are they really non-equal? If they are, you should potentially just use the default implementation of Equals/GetHashCode which uses reference identity for equality. Where you would use two objects with the same ID, you just use two references to the same object instead.

It really all depends on the context, and you haven't given much of that - but adding an ID just for equality is a bit of a design smell.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Thanks John - what I would do without the ID would be to disallow two objects with equal properties in the same List collection. But the goal with the List.Remove method would be the following - the list is a property on another type, call it Owner. An instance of Owner and his List are persisted to storage. Later, they can be retrieved, and one car can be removed. I was using the ID to identify the one to remove, but now that you mention it, reference equality will work for that too, since the caller has to instantiate the list before removing anyway ... thanks for helping me see. – Andrew B Schultz May 28 '12 at 16:03
1

Instead of implementing Equals and GetHashCode just use RemoveAll:

myList.RemoveAll(x => x.ID == myCar.ID);

This allows you to specify a predicate that indicates what items should be removed instead (it doesn't matter that you are only removing one item).

Implementing Equals and GetHashCode in the way you describe strikes me as extremely dodgey - if your Equals implementation returns true then your GetHashCode method needs to return the same value so that those two objects will be placed in the same bucket in a hashtable. Your implementation (as I understand it) doesn't match this criteria as the base GetHashCode implementation is almost certainly going to return different values for two Car instances, regardless of if they have the same ID or not.

Implementing Equals and GetHashCode isn't entirely trivial and is probably something I'd generally avoid doing if there are alternatives. If you really want to do this then take a look at these resoruces:

Also hash codes are not GUIDs

Community
  • 1
  • 1
Justin
  • 84,773
  • 49
  • 224
  • 367
  • Which part of my question suggested that I thought a hashcode was a guid? – Andrew B Schultz May 28 '12 at 16:13
  • @AndrewBSchultz "Is this a good approach, or is implementing a GUID for a small class too heavy-handed?", also the Title :) – Justin May 28 '12 at 16:17
  • OK. I know a GUID isn't a hash code. The GUID was meant to be used in the Equals() implementation, and the GetHashCode() override was, in my mind, simply a requirement that grew out of overriding the Equals() method. I used base.GetHashCode because I didn't know how to construct an int that would be unique from my properties - that's why I said "there are not int properties on my car type." – Andrew B Schultz May 28 '12 at 16:27
  • 1
    @AndrewBSchultz See the answer for "What is the best algorithm for an overridden System.Object.GetHashCode?", you can combine hash codes from other objects (e.g. strings) to create a unique hash code, e.g. `ID.GetHashCode()` where `ID` is your guid. – Justin May 28 '12 at 16:59