HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Overriding GetHashCode and Equals

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gethashcodeandoverridingequals

Problem

I'm creating a class to wrap a list of Mask and I'd like to know if I'm overriding the GetHashCode() and Equals() correctly.

Mask is a wrapper for a List of Points. It does implement IEnumerable.

public class MaskCollection : IEnumerable
    {
        #region Variables

        private List masks;

        public int Count { get { return masks.Count; } }

        #endregion

        #region Overrides

        public override int GetHashCode()
        {
            int hash = 0;

            foreach(Mask mask in masks)
            {
                hash ^= mask.Center.GetHashCode();
            }

            return hash;
        }

        public override bool Equals(object obj)
        {
            if (ReferenceEquals(this, obj)) return true;

            if (!(obj is MaskCollection)) return false;

            MaskCollection other = (MaskCollection)obj;
            if (this.Count != other.Count) return false;

            HashSet uniqueMasks = new HashSet(masks);
            foreach(Mask mask in other)
            {
                if (!uniqueMasks.Contains(mask)) return false;
            }

            return true;
        }

        #endregion
    }


And if I am overloading them correctly, can this code be improved? My Equals() looks really slow, if they actually are equal.

Solution

This implementation of Equals violates the contract of Equals.

From MSDN:


The following statements must be true for all implementations of the
Equals(Object) method. In the list, x, y, and z represent object
references that are not null.



  • ...



  • x.Equals(y) returns the same value as y.Equals(x).




The problem is with this code

HashSet uniqueMasks = new HashSet(masks);
foreach(Mask mask in other)
{
    if (!uniqueMasks.Contains(mask)) return false;
}

return true;


As it's possible that all masks in other are in uniqueMasks, but uniqueMasks contains a mask not in other (i.e. other \$\subsetneq\$ uniqueMasks).

For example, the following code will print False True.

var x = new MaskCollection(new[] { new Mask(0), new Mask(0) });
var y = new MaskCollection(new[] { new Mask(0), new Mask(1) });
Console.WriteLine(x.Equals(y));
Console.WriteLine(y.Equals(x));


Given this implementation of Mask

public class Mask : IEquatable
{
    private readonly int center;

    public Mask(int center)
    {
        this.center = center;
    }

    public int Center
    {
        get { return this.center; }
    }

    public bool Equals(Mask other)
    {
        return this.center == other.center;
    }

    public override int GetHashCode()
    {
        return this.center.GetHashCode();
    }
}


You could write

return new HashSet(masks).SetEquals(other);


but I would probably recommend not overriding GetHashCode and Equals for this class, if you don't have a good reason to do so.

Code Snippets

HashSet<Mask> uniqueMasks = new HashSet<Mask>(masks);
foreach(Mask mask in other)
{
    if (!uniqueMasks.Contains(mask)) return false;
}

return true;
var x = new MaskCollection(new[] { new Mask(0), new Mask(0) });
var y = new MaskCollection(new[] { new Mask(0), new Mask(1) });
Console.WriteLine(x.Equals(y));
Console.WriteLine(y.Equals(x));
public class Mask : IEquatable<Mask>
{
    private readonly int center;

    public Mask(int center)
    {
        this.center = center;
    }

    public int Center
    {
        get { return this.center; }
    }

    public bool Equals(Mask other)
    {
        return this.center == other.center;
    }

    public override int GetHashCode()
    {
        return this.center.GetHashCode();
    }
}
return new HashSet<Mask>(masks).SetEquals(other);

Context

StackExchange Code Review Q#79739, answer score: 6

Revisions (0)

No revisions yet.