CODEWITHSUNDEEP
c#-3.0
Mark
Mark

Reputation:

Equals and GetHashCode

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();

        }

Upvotes: 4

Views: 341

Answers (3)

Jon Skeet
Jon Skeet

Reputation: 1499760

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;
    }
}

Upvotes: 4

JaredPar
JaredPar

Reputation: 754515

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.

Upvotes: 4

Marc Gravell
Marc Gravell

Reputation: 1062492

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...

Upvotes: 0

Related Questions