views:

235

answers:

2

I'm new to C#. Perhaps I'm not implementing IEquatable properly, because objects of my type that should be considered the same are not.

The class:

class CompPoint : IComparable {
        public int X;
        public int Y;

        public CompPoint(int X, int Y) {
            this.X = X;
            this.Y = Y;
        }

   public override bool Equals(Object o) {
        if (!(o is CompPoint)) {
            throw new ArgumentException(String.Format("argument is not a CompPoint (%s given)", o));
        }
        CompPoint cp = (CompPoint)o;

        return this.X == cp.X && this.Y == cp.Y;
    }

    public override int GetHashCode() {
        int hash = base.GetHashCode(); // this is a problem. replace with a constant?
        hash = (hash * 73) + this.X.GetHashCode();
        hash = (hash * 73) + this.Y.GetHashCode();
        return hash;
    }
}

(There is more to CompPoint than this that does justify it being a class.)

Then, this test fails:

    [TestMethod()]
    public void compPointTest() {
        Assert.AreEqual(new CompPoint(0, 0), new CompPoint(0, 0));
    }

What am I misunderstanding? Is Assert.AreEqual() using referential equality? Is my Equals() function in CompPoint messed up?

This function also fails:

    public void EqualsTest() {
        Assert.IsTrue(new CompPoint(1, 1).Equals(new CompPoint(1, 1)));
    }

This reason for this is that I'm using a Dictionary, and it's not working the way I'd hope it would:

    [TestMethod()]
    public void dictCompPointTest() {
        IDictionary<CompPoint, int> dict = new Dictionary<CompPoint, int>();
        dict[new CompPoint(0, 0)] = 4;
        dict[new CompPoint(0, 0)] = 24;
        dict[new CompPoint(0, 0)] = 31;

        Assert.AreEqual(31, dict[new CompPoint(0, 0)]);
        Assert.AreEqual(1, dict.Count);
    }

The test fails with this message:

Test method ShipAILabTest.BoardUtilsTest.dictCompPointTest threw exception: System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary.

This test encapsulates my expectations. I am hoping that since the key is identical each time, the value will be overwritten. What is the Dictionary using to test for equality?

UPDATE: I added an equality function, as per Thomas's suggestion, and now the CompPoint comparison tests work, and dictCompPointTest works.

    public override bool Equals(Object o) {
        if (!(o is CompPoint)) {
            throw new ArgumentException(String.Format("argument is not a CompPoint (%s given)", o));
        }
        CompPoint cp = (CompPoint)o;

        return this.X == cp.X && this.Y == cp.Y;
    }

Mysteriously, this test still fails:

   [TestMethod()]
    public void dictCPTest2() {
        IDictionary<CompPoint, int> dict = new Dictionary<CompPoint, int>();
        dict[new CompPoint(2, 2)] = 2;
        dict[new CompPoint(2, 2)] = 2;

        Assert.AreEqual(1, dict.Count);
    }

The test also fails when the keys are new CompPoint(4, 1), but not when the keys are new CompPoint(0, 1). Why could this be working for some values and not others?

More mystery: the hash code function seems to be working pretty poorly. This test fails:

    [TestMethod()]
    public void hashCodeTest() {
                int x = 0;
                int y = 0;
                Assert.AreEqual(new CompPoint(x, y).GetHashCode(), new CompPoint(x, y).GetHashCode());    
    }

The hash code function is listed above. What is the problem here? Shouldn't the two CompPoint objects have the same hash code? Maybe my call to base.getHashCode() is a problem?

+3  A: 

I think Assert.AreEqual just uses Object.Equals, not IEquatable<T>.Equals. So you need to override Equals to reflect the logic of IEquatable<T>.Equals.

Or you could also use Assert.IsTrue :

IEquatable<CompPoint> p1 = new CompPoint(0, 0);
IEquatable<CompPoint> p2 = new CompPoint(0, 0);
Assert.IsTrue(p1.Equals(p2));

Note that I declare p1 and p2 as IEquatable<CompPoint> : this it to ensure that IEquatable<CompPoint>.Equals is called rather than Object.Equals, since the interface is implemented explicitly

EDIT: by the way, you might want to declare CompPoint as a struct rather than a class. That way, you don't even have to implement anything, since value types are compared according to their field values

Thomas Levesque
+1. Beat me to the punch. Instead of having `Equals()` duplicate the logic, though, just have it call the other method.
John Feminella
Updated my question a bit. Is it important to use `IEquatable<CompPoint>` as the type, instead of just `CompPoint`?
Rosarch
It is important because you implemented `IEquatable<CompPoint>` **explicitly**. However I don't understand why it doesn't work when you use your type as a dictionary key :S
Thomas Levesque
+1  A: 

If you're overriding Equals then you should also override GetHashCode, as this is what the dictionary will use in the first instance to determine if two keys match. (Any two objects of the same type that are considered equal should return the same value from GetHashCode.)

public class CompPoint : IEquatable<CompPoint>
{
    // ...

    public override bool Equals(object obj)    // object
    {
        return this.Equals(obj as ComPoint);
    }

    public bool Equals(CompPoint other)    // IEquatable<ComPoint>
    {
        return !object.ReferenceEquals(other, null)
            && this.X.Equals(other.X)
            && this.Y.Equals(other.Y);
    }

    public override int GetHashCode()    // object
    {
        int hash = 5419;
        hash = (hash * 73) + this.X.GetHashCode();
        hash = (hash * 73) + this.Y.GetHashCode();
        return hash;
    }
}
LukeH
This is intriguing, yet ultimately unfruitful. The `Dictionary` still fails to detect the duplicates.
Rosarch
I think `getHashCode()` may be causing the problem. (see above)
Rosarch
Don't know if you fixed this or not, but if your GetHashCode method is still what you have posted in the code above, it will still generate a different has for two instances (i.e. two new CompPoint(0, 0)) due to them calling base.GetHashCode()
jeffora
@Rosarch: Your call to `base.GetHashCode` *is* the problem. The default implementation of `GetHashCode` will return a different hashcode for different instances of a reference type, even if those instances contain the same data. You're using the default hashcode as the "seed" of your own implementation, so your method will also return different hashcodes for different instances.
LukeH