30

In an example, my professor has implemented Equals as follows:

public class Person {
    private string dni;

    // ...

    public override bool Equals(object o) {
        if (o == null)
            return (this == null);
        else {
            return ((o is Person) && (this.dni == (o as Person).dni));
        }
    }
}

I have no experience with C#, but as far as I know this cannot be null inside a member function (at least this is true in C++ and Java, the languages I know) so the if seems weird to me.

Am I right or is there any component in c# I dont know of which makes the test this == null necesary?

DaveDev
  • 41,155
  • 72
  • 223
  • 385
José D.
  • 4,175
  • 7
  • 28
  • 47
  • 5
    You should have asked your professor this, to see what his reasoning was. It could have been a learning experience for both of you! – DaveDev Oct 10 '13 at 15:39
  • 9
    If you assume that "this" can be null, then the else statment should check if this is null. – the_lotus Oct 10 '13 at 15:42
  • 6
    Jon Skeet has something to say: http://stackoverflow.com/a/3143558/393487 – Ahmed KRAIEM Oct 10 '13 at 15:49
  • I would just replace the if block by `if(o==this) return true;`. – thersch Oct 10 '13 at 16:12
  • @thersch are you trolling or being serious? – MattDavey Oct 10 '13 at 16:35
  • @MattDavey I mean the if block only, not the else block. – thersch Oct 10 '13 at 16:46
  • 2
    @AustinSalonen: Your statement is incorrect; in C# null is equal to null, whether the nulls compared are reference or nullable value types. You may be getting confused with Visual Basic, in which `Null = Null` produces neither true nor false, but another null. Or you might be getting confused with `NaN`, which does have the property that `Nan == NaN` is false. (As is `Nan != NaN`). – Eric Lippert Oct 10 '13 at 17:32
  • Of course it can be `null`! http://xacc.wordpress.com/2009/12/07/calling-an-instance-method-in-the-clr-without-an-instance/ . IMG: http://xacc.files.wordpress.com/2008/10/time-to-stop.png – leppie Oct 10 '13 at 17:49
  • @EricLippert: You are correct. I've removed my comment. – Austin Salonen Oct 10 '13 at 18:18
  • 2
    Since this guy/gal may be a relatively new programmer, and possibly overwhelmed by the complexity of some of these responses, has anyone highlighted the fact that the short (and almost correct, with caveats, details, and exceptions explained by the answers below) answer is "no, 'this' cannot be null"? I think someone might lose the forest for the trees here. – Beska Oct 10 '13 at 21:12
  • (I say the above because I have seen someone check for this == null in code, and it wasn't for any good reason.) – Beska Oct 10 '13 at 21:17

4 Answers4

58

I have no experience with C#, but as far as I know this cannot be null inside a member function (at least this is true in C++ and Java, the languages I know)

Let's begin by noting that your statement is false.

In C++, dispatching a method on a null receiver is undefined behavior and undefined behavior means that anything can happen. "Anything" includes the program passing NULL as this and continuing as though nothing was wrong. Of course it is somewhat silly to check whether this is null in C++ because the check can only be true if you already do not know what your program is doing, because its behavior is undefined.

Whether this can be null in Java I have no idea.

Now to address your question about C#. Let's assume that == is not overloaded. We'll come back to this point later.

Your method is written in C#. Suppose it is invoked from a C# program with a null receiver. The C# compiler evaluates whether the receiver could possibly be null; if it could possibly be null then it ensures that it generates code that does a null check before invoking the method. Therefore this check is pointless in that scenario. This is of course the 99.9999% likely scenario.

Suppose it is invoked via Reflection, as in mike z's answer. In that case it is not the C# language that is performing the invocation; rather, someone is deliberately abusing reflection.

Suppose it is invoked from another language. We have a virtual method; if it is invoked from this other language with virtual dispatch then a null check must be performed, because how else could we know what is in the virtual slot? In that scenario it cannot be null.

But suppose it is invoked from another language using non-virtual dispatch. In that case the other language need not implement the C# feature of checking for null. It could just invoke it and pass null.

So there are several ways in which this could be null in C#, but they are all very much out of the mainstream. It is therefore very rare for people to write code as your professor has. C# programmers idiomatically suppose that this is not null and never check for it.

Now that we've gotten that out of the way, let's criticize that code some more.

public override bool Equals(object o) {
    if (o == null)
        return (this == null);
    else {
        return ((o is Person) && (this.dni == (o as Person).dni));
    }
}

First off there is an obvious bug. We presume that this could be null, ok, let's run with that. What stops this.dni from throwing null reference exception??? If you're going to assume that this can be null then at least do so consistently! (At Coverity we refer to this sort of situation as a "forward null defect".)

Next: we are overriding Equals and then using == inside, presumably to mean reference equality. This way lies madness! Now we have a situation where x.Equals(y) can be true but x==y can be false! This is horrid. Please don't go there. If you're going to override Equals then overload == at the same time, and implement IEquatable<T> while you're at it.

(Now, there is a reasonable argument to be made that madness lies in either direction; if == is consistent with Equals with value semantics then personx == persony can be different than (object)personx == (object)persony, which seems strange also. The takeaway here is that equality is pretty messed up in C#.)

Moreover: what if == is overridden later? Now Equals is calling an overridden == operator, when the author of the code clearly wishes to be doing a reference comparison. This is a recipe for bugs.

My recommendations are (1) write one static method that does the right thing, and (2) use ReferenceEquals every time there could possibly be any confusion over what kind of equality is meant:

private static bool Equals(Person x, Person y)
{
    if (ReferenceEquals(x, y))
        return true;
    else if (ReferenceEquals(x, null))
        return false;
    else if (ReferenceEquals(y, null))
        return false;
    else 
        return x.dni == y.dni;
}

That nicely covers every case. Notice that it is crystal clear to the reader when reference equality semantics are meant. Also note that this code makes it very easy to put breakpoints on each possibility, for debugging purposes. And finally, notice that we take the cheapest possible early out; if the objects are reference equal then we don't have to do the potentially expensive comparison of the fields!

Now the other methods are easy:

public static bool operator ==(Person x, Person y) 
{
  return Equals(x, y);
}
public static bool operator !=(Person x, Person y) 
{
  return !Equals(x, y);
}
public override bool Equals(object y)
{
  return Equals(this, y as Person);
}
public bool Equals(Person y)
{
  return Equals(this, y);
}

Notice how much more elegant and clear my way is than your professor's way. And notice that my way handles a null this without ever comparing this to null directly.

Again: all of this illustrates that the compromise position arrived at, in which both value and reference equality are possible and there are four (==, !=, object.Equals(object) and IEquatable<T>.Equals(T)) ways to implement equality, is very complicated and confusing even without assuming that this can or cannot be null.

If this subject interests you, I describe a slightly harder problem on my blog this week: how to implement comparisons in general, including inequalities.

http://ericlippert.com/2013/10/07/math-from-scratch-part-six-comparisons/

The comments are particularly interesting as a critique of how C# handles equality.

Finally: don't forget to override GetHashCode. Make sure you do it right.

Cyril Gandon
  • 16,830
  • 14
  • 78
  • 122
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 1
    I read through a thoroughly detailed answer... of course it ends up being Eric Lippert. This is why I can't find any questions to answer! – Kyle W Oct 10 '13 at 17:02
  • 1
    The _one static method that does the right thing_ is definitely the way to go. Simple, readable, efficient, no `==` inside `Equals` nor `Equals` inside `==` override, that's just *perfect*. +1 – ken2k Oct 10 '13 at 17:12
  • 4
    "And notice that my way handles a null this" is not really true for the override, since if "this" were null, it would compare equal to a non-null object that isn't a Person (since the "as" operator would return null). – Cyanfish Oct 10 '13 at 17:52
  • @Cyanfish: Good point, I hadn't considered that one! Since it is unlikely to arise in practice I am not particularly concerned. – Eric Lippert Oct 10 '13 at 18:00
  • @KyleW If it wasn't for "Whether this can be null in Java I have no idea." early on it could've been John Skeet instead. – Dan Is Fiddling By Firelight Oct 10 '13 at 18:10
  • in java you can't have a null `this`; with reflection you get a nullpointerexception before it calls the method, I suspect the jni path checks as well – ratchet freak Oct 10 '13 at 22:18
  • I amend my statement: jni does not check for null when invoking through jni, but [it just about says it's undefined behavior](http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17593) – ratchet freak Oct 10 '13 at 22:28
  • Given that operators are bound statically while `Equals` is bound virtually, there's often no way to create consistently-matching non-default behaviors for `==` and `Equals`. Letting `==` implement the default reference-equivalence relation is apt to be less astonishing than having it test for something which is sorta like `Equals(Object)` but is not an equivalence relation (e.g. because one could have x==y and y==z, but x!=z). – supercat Oct 13 '13 at 17:05
  • I'm curious, though: what would you think of having a class `MyType` which does override `==(MyType, MyType)` also provide overrides for `==(MyType, Object)` and `==(Object, MyType)`, both of which are tagged `[Obsolete("Cast operands to (myType) or (Object)",true);]`? I've not seen any types do that, but it would seem to help prevent some "astonishing" cases. – supercat Oct 13 '13 at 17:10
13

Yes, in some cases. The most common case is if you invoke an instance method using a delegate created by reflection and pass a null receiver. This will print "True":

public class Test
{
    public void Method()
    {
        Console.WriteLine(this == null);
    }
}

public class Program
{
    public static void Main(string[] args)
    {
        object t = new Test();
        var methodInfo = t.GetType().GetMethod("Method");
        var a = (Action)Delegate.CreateDelegate(typeof(Action), null, methodInfo);
        a();
    }
}

Honestly, though I've never seen someone actually check thisfor null in production code.

Mike Zboray
  • 39,828
  • 3
  • 90
  • 122
  • +1 for an example of when this could be null. Anyway, if that's why the `this==null`is there, it should be checked in the else too, so the code is wrong anyway. – José D. Oct 10 '13 at 15:49
  • 1
    @Trollkemada As someone who is learning the language, you should carefully read Eric Lippert's answer. It has a canonical implementation of `Equals` that you should try to emulate. – Mike Zboray Oct 10 '13 at 16:12
2

One way this could == null is if you practiced some black magic by overloading the == operator. For example, if someone decides that having a null or empty-string dni is equivalent to a null person:

public static bool operator ==(Person a, Person b)
{
    if(String.IsNullOrEmpty(a.dni) && (object)b == null)
        return true;

    if(String.IsNullOrEmpty(b.dni) && (object)a == null)
        return true;

    return Object.ReferenceEquals(a, b);
}

Note this is probably a Really Bad Idea.

lc.
  • 113,939
  • 20
  • 158
  • 187
2

Yes, it is possible and in fact not entirely unlikely. The C# language gives an excellent guarantee that no method can be called on a null reference to a class object, it generates code that makes a null check at the call site. Which certainly does avoid a lot of head-scratching, a NullReferenceException inside the method can be pretty hard to debug without that check.

That check is however specific to C#, not all .NET languages implement it. The C++/CLI language doesn't for example. You can see it for yourself by creating a solution with a C++ CLR Console mode project and a C# class library. Add a reference in the CLR project to the C# project.

C# code:

using System;

namespace ClassLibrary1 {
    public class Class1 {
        public void NullTest() {
            if (this == null) Console.WriteLine("It's null");
        }
    }
}

C++/CLI code:

#include "stdafx.h"

using namespace System;

int main(array<System::String ^> ^args) {
    ClassLibrary1::Class1^ obj = nullptr;
    obj->NullTest();
    Console::ReadLine();
    return 0;
}

Output:

It's null

This is not otherwise undefined behavior or illegal in any way. It is not verboten by the CLI spec. And as long as the method doesn't access any instance members then nothing goes wrong. The CLR handles this as well, a NullReferenceException is also generated for pointers that are not null. Like it will be when NullTest() accesses a field that isn't the first field in the class.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536