C# -- Is this checking necessary " obj is Person && obj != null"

Question

I saw the following code,

public override bool Equals(object obj)
{
  // From the book http://www.amazon.co.uk/Pro-2010-NET-4-0-Platform/dp/1430225491
  // Page 254!
  if (obj is Person && obj != null)
...
}

Based on my understanding, I think the code should be rewritten as follows:

public override bool Equals(object obj)
{
  if (obj is Person)
...
}

Is that correct?

Based on http://msdn.microsoft.com/en-us/library/scekt9xw%28v=vs.80%29.aspx

An is expression evaluates to true if the provided expression is non-null, and the provided object can be cast to the provided type without causing an exception to be thrown.

I think the extra checking for null is NOT necessary at all. In other words, that code "obj != null" should never be hit at all.

Thank you

// Updated //

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ConsoleApplication2
{
    class Employee
    {
        public static void CheckIsEmployee(object obj)
        {
            if (obj is Employee)
            {
                Console.WriteLine("this is an employee");
            }
            else if (obj == null)
            {
                Console.WriteLine("this is null");
            }
            else
            {
                Console.WriteLine("this is Not an employee");
            }
        }   
    }

    class NotEmployee
    { }

    class Program
    {
        static void Main(string[] args)
        {
            Employee e = new Employee();

            Employee.CheckIsEmployee(e);

            Employee f = null;
            Employee.CheckIsEmployee(f);

            NotEmployee g = new NotEmployee();
            Employee.CheckIsEmployee(g);
        }
    }
}

Output results:

this is an employee
this is null
this is Not an employee

Answer 1

My preference would be to use the as keyword.

public override bool Equals(object obj)
{
    var objectToCompare = obj as Person;
    if ( objectToCompare == null )
        return false;
    ...
}

The advantage is that later in the method, you have a typed instance (objectToCompare) with which to do your comparisons.

You are correct in your assessment that obj is Person will return false if obj is not derived from Person or if obj is null and thus obj is Person && obj != null is redundant; you only need obj is Person if you are using that style. Technically, there might be a fractional performance gain by checking for null first, but the gain would be negligible.

Answer 2

Functionally it is correct, but it is quicker to check for null than to do a runtime type check, so you're better off checking for null first in most situations. That way, if obj is null, the overhead of the runtime type check will not be incurred.

Answer 3

Your version looks more correct to me. It won't be a Person unless it's non-null, so the obj != null is redundant.

Answer 4

A reference to a Person can still be a null reference, so technically yes, both checks are required.

I like Thomas' answer as to how to handle both checks at once. null as MyClass == null, and myClassInstance as OtherClass == null, so with one check of the safely-cast object you've confirmed both conditions, and as he said, you have a strongly-typed reference to work with from then on.

There is an interesting discussion of the difference at low levels between is and as keyword operations. Google "Is as is or is is as" (I'm having Internet trouble at present). It turns out they work very similarly at the IL level.