4

What do you think about this Person class? Is it a bad idea or best practise to override Equals and GetHashCode like that? 

public class Person { public int PersonId { get; set; } public string Name { get; set; } public override bool Equals(object obj) { var person = obj as Person; return PersonId == person.PersonId; } public override int GetHashCode() { return PersonId; } }

Usage :

 static void Main(string[] args) { var list = new List<Person>(); list.Add(new Person(){ PersonId = 1, Name = "Mike"}); list.Add(new Person() { PersonId = 2, Name = "Michael Sync" }); list.Add(new Person(){ PersonId = 1, Name = "Mike"}); var list1 = new List<Person>(); list1.Add(new Person() { PersonId = 1, Name = "Mike" }); list1.Add(new Person() { PersonId = 3, Name = "Julia" }); var except = list.Except(list1); foreach (var item in except) { Console.WriteLine(item.Name); } Console.ReadKey(); }
Improve this question

3 Answers 3

Reset to default
4

A few points:

  1. It's not null safe or "different type" safe. Try this:

    new Person().Equals(new Object());

    or

    new Person().Equals(null);

    Bang.

  2. Classes defining equality operations should usually be immutable IMO. Changing the contents of an object after using it as a dictionary key is a Bad Thing, for example.

  3. Consider implementing IEquatable<Person>

A quick reimplementation, which still assumes you want equality based solely on ID.

public sealed class Person : IEquatable<Person> { private readonly int personId; public int PersonId { get { return personId; } private readonly string name; public string Name { get { return name; } } public Person(int personId, string name) { // Is a null name valid? If not, throw here. this.personId = personId; this.name = name; } public override bool Equals(object obj) { return Equals(obj as Person); } public Equals(Person other) { return other != null && other.personId == personId; } public override int GetHashCode() { return personId; } }
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Is GetHashCode basically the Jon Skeet bat signal?
So, the most of our UI entity need to be implemented IEquatable then. Thanks a lot for suggesting.
4

Yes this is wrong. You should never use a mutable property as part of the calculation for GetHashCode. Doing so opens you up to numerous hard to track down bugs.

Improve this answer

1 Comment

Thanks. As of now, our UI entity class are not overriding those properties but if we are using Except or etc then it;s working.. thanks for suggestion.
0

One problem I can see is that you'll get lots of collisions for new (unsaved) records (lots of zeros) - unless you do something like have consecutive -ve ids for those... But do you really need to use Person as a key? Personally I don't think I'd bother...

Improve this answer

2 Comments

Actually, I want to use LINQ Except or other functions. Currently, we are not overriding anything so we cant use Except in our code. so, am looking for the best practice to implement something in our Entity classes.
Most of the time you can solve those problems better using a selector, and comparing directly on an existing property (the id, for example)