Non-readonly fields referenced in GetHashCode()

user_1360251 0
Viewed 18164

Started with overriding concepts and I override the methods Equals and GetHashCode.

Primarily I came up with this "very simple code":

internal class Person
    {
        public string name;

        public int age;

        public string lname;

        public Person(string name, int age, string lname)
        {
            this.name = name;
            this.age = age;
            this.lname = lname;
        }

        public override bool Equals(object obj)
        {
            var person = obj as Person;
            if (person != null)
            {
                return person.age == this.age && person.name == this.name && person.lname == this.lname;
            }

            return false;
        }

        public override int GetHashCode()
        {
            return this.age.GetHashCode() * this.name.GetHashCode() * this.lname.GetHashCode();
        }
    }

While this works great, my "co-developer" Mr.Resharper gave me some suggestions:

  1. Non-readonly fields referenced in GetHashCode(). Suggestions came in this line of code:

return this.age.GetHashCode() * this.name.GetHashCode() * this.lname.GetHashCode();

  1. Should we use GetHashCode only for Properties?
.net-4.0 .net c#
3 Answers

If you change the value of a field, used in the hash calculation, after the object had been added to a hash based container like Dictionary or HashSet, you are essentially breaking the inner state of the container. Why is that? Because the object had been stored in a bucket corresponding to a hash value based on its initial state. When the state is changed, e.g. 'age' is modified, the object will continue to live in its old bucket in the hash container although this is not the correct bucket based on its current hash code. This can lead to pretty messy behaviour and a lot of headaches. I've written an article on this topic with some very specific examples, so you may want to check it out.

user_32701390

I don't really like the idea of having directly public fields this way.

I would prefer properties, and unless I really needed to change them, I'd limit access to them using the relatively new init feature from c# 9:

// Will only be setable from the constructor. This will effectively make the
// values read-only (and allow them to be used to generate hashes without
// resharper complaining).
public string name {get; init; } 
public string lname {get; init; }
    
// Note: I'd also use an unchanging date for birth:
public DateTime BirthDate {get; init; };

// Add logic below to actually calculate the age here based 
// on the diff between current date and birth date above.
public int Age {
    get { return ... };
    private set;
};
user_3366480
Related