Operator overloading giving error

Question

I have a class named Point that overloading "==" and "!=" operators to compare two Point object. How can I compare my Point object with "null", this a problem because when I call == or != operator with null a problem inside Equals method. Please open a console application and see what I want to say.How can I fix it.

public class Point
    {
        public int X { get; set; }
        public int Y { get; set; }


        public static bool operator == (Point p1,Point p2)
        {
            return p1.Equals(p2);
        }

        public static bool operator != (Point p1, Point p2)
        {
            return !p1.Equals(p2);
        }


        public override bool Equals(object obj)
        {
            Point other = obj as Point;

            //problem is here calling != operator and this operator calling  this method again
            if (other != null)
            {
                if (this.X == other.X && this.Y == other.Y)
                {
                    return true;
                }
                return false;
            }

            else
            {
                throw new Exception("Parameter is not a point");
            }
        }
    }

class Program
    {
        static void Main(string[] args)
        {
            Point p1 = new Point { X = 9, Y = 7 };
            Point p2 = new Point { X = 5, Y = 1 };

            p1.X = p2.X;
            p1.Y = p2.Y;

            bool b1=p1==p2;

            Console.ReadKey();
        }
    }

Answer 1

Use ReferenceEquals to check for null:

if (ReferenceEquals(other, null))

Having said that, Equals should generally not throw an exception if it encounters an unknown object type, it should just return false, thus here is the method I would write:

public override bool Equals(object obj)
{
    Point other = obj as Point;

    if (ReferenceEquals(other, null))
        return false;

    return (this.X == other.X && this.Y == other.Y);
}

An additional problem is that your operators will throw an exception if you compare null against something, as you can't invoke the instance method on a null reference.

Thus, here is the full class I would've written:

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }


    public static bool operator == (Point p1,Point p2)
    {
        if (ReferenceEquals(p1, p2)) return true;
        if (ReferenceEquals(p1, null)) return false;
        return p1.Equals(p2);
    }

    public static bool operator != (Point p1, Point p2)
    {
        return !(p1 == p2);
    }

    public bool Equals(Point other)
    {
        if (ReferenceEquals(other, null))
            return false;

        return (this.X == other.X && this.Y == other.Y);
    }

    public override bool Equals(object obj)
    {
        Point other = obj as Point;
        if (ReferenceEquals(other, null))
            return false;
        return Equals(other);
    }
}

Answer 2

You should not throw an exception at:

        if (other != null)
        {
            ...
        }
        else
        {
            throw new Exception("Parameter is not a point");
        }

You can read http://msdn.microsoft.com/en-US/library/336aedhh%28v=vs.85%29.aspx to see how Equals() is properly implemented.

Answer 3

Handling equality in C# can be tricky and it's easy to fall into pit traps like you've encountered here.

I always follow the same pattern when dealing with equality, be it in value types or in reference types (except the null checking obviously): implement a static private helper method that takes care of all possibilities, and let all equality checks invoke this method.

So, in your case, I'd do the following:

UPDATE: Fixed a typo, equals should be private, not public.

 private static bool equals(Point p1, Point p2)
 {
     if (object.ReferenceEquals(p1, p2))
         return true;

     if (object.ReferenceEquals(p1, null) || object.ReferenceEquals(p2, null))
         return false;

     return p1.X == p2.X && p1.Y == p2.Y;
 }

Ok, so what are we doing here?

1. We first check to see if both objects have equal references. If thats the case, then they have to be equal as they are the same object. Note that this takes care of the null == null case.

2. Then we check to see if any of the two arguments is equal to null. If they are, then we know that p1 and p2 are not equal.

   To see if the arguments are equal to null we use the satic method bool object.ReferenceEquals(,) to avoid the issue you are having which is calling again your equality implementation.

3. Last, we now know we have two non null Point arguments so we go ahead and implement the specific equality logic of two Point objects.

With this simple method we have dealt with all possible equality checks in our class. Now we simply have to invoke this method from all possible equality methods and operators:

public static bool operator ==(Point p1, Point p2)
{
    return Point.equals(p1, p2);
}

public static bool operator !=(Point p1, Point p2)
{
    return !Point.equals(p1, p2);
}

public override bool Equals(object obj)
{
    return Point.equals(this, obj as Point);
}

Additionaly, you should implement the IEquatable<Point> interface which is pretty straightforward. This is specially relevant when dealing with value types as you avoid boxing conversions when performing equality checks:

public bool Equals(Point p)
{
    return Point.equals(this, p);
}

And last but not least, you are overriding the Equals method and the == and != operators so you should also override int GetHashCode() in a way consistent with your equality implementation: if p1 == p2 then p1.GetHashCode() must equal p1.GetHashCode() (Note, that this does not mean that if p1 != p2, p1.GetHashCode() must not equal p2.GetHashCode()):

public override int GetHashCode()
{
    return this.X ^ this.Y;
}

Hope this small guideline helps.

Answer 4

Maybe something like this:

public override bool Equals(object obj)
{
    if (obj != null && obj is Point)
    {
       Point other = (Point)obj;
       if (this.X == other.X && this.Y == other.Y)
       {
           return true;
       }
       return false;
    }
    return false;
}