5
\$\begingroup\$

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

public class MaskCollection : IEnumerable<Mask>
    {
        #region Variables

        private List<Mask> 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<Mask> uniqueMasks = new HashSet<Mask>(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.

\$\endgroup\$
6
\$\begingroup\$

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<Mask> uniqueMasks = new HashSet<Mask>(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<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();
    }
}

You could write

return new HashSet<Mask>(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.

\$\endgroup\$
  • \$\begingroup\$ Nice! I didn't notice that. Already implemented your solution. Sadly I think I do need, since I intend to use it in HashSets. \$\endgroup\$ – Trauer Feb 6 '15 at 15:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.