-3

Should we check if variable is null before setting it to null?

if (MyBills != null) 
{
    MyBills = null;
}

For, example, in a Java related question the performance implications are minimal. Is this the case in C#? Other implications?

Testing

I've created the following code to test:

var watch = System.Diagnostics.Stopwatch.StartNew();

int iterations = int.MaxValue;
List<int> myBills= null;
for (var i = 0; i < iterations; i++)
{
    if (myBills!= null)
    {
        myBills = null;
    }
}
watch.Stop();
var elapsedMs = watch.ElapsedMilliseconds;
Console.WriteLine(elapsedMs);

Running it on rextester with and without the if (myList != null) the results are as following:

With check      Without check
988             941
938             1021
953             987
998             973
1004            1031

Average 
976.2           990.6

So, even testing it in a non-controlled environment, the performance implications are irrelevant.

Community
  • 1
  • 1
Mario Levrero
  • 3,345
  • 4
  • 26
  • 57

3 Answers3

13

No, there is not much use. Probably checking the variable being null or not is just as expensive as setting it to null one time too many.

If it was a property, with additional logic behind it, it could make sense to test it before, but that should actually be the responsibility of the logic in the property, not your code.

Patrick Hofman
  • 153,850
  • 22
  • 249
  • 325
  • Thanks, Patrick. I don't get why so many downvoters for the question. – Mario Levrero Feb 23 '17 at 13:27
  • 6
    Maybe they'd expected a little more effort in asking, like now you linked to Java as comparison. You could also have ran some tests. – Patrick Hofman Feb 23 '17 at 13:28
  • Patrick, I've updated the question with some tests as you suggested. I think that only with constructive critics as you did we can make a better S.O. Thanks again – Mario Levrero Feb 23 '17 at 13:51
  • You are welcome. Indeed, it doesn't care too much as you see in the test. although there are better testing methods, which will yield a more accurate result. – Patrick Hofman Feb 23 '17 at 13:52
5

Aside from the property/setter argument I can't see any logical reason for this in the code you have provided.

However, if you want to perform extra operations on the object before setting it to null it makes sense:

if (MyBills != null)
{
     MyBills.Dispose();
     MyBills = null;
}

Or perform some other operation like logging a result.

But these two examples are both outside of the example code you have provided.


Because people seem to be questioning the use case for my first example, here's one. This will prevent multiple calls to Dispose causing an ObjectDisposedException.

public class MyClass : IDisposable
{
    private SomeDisposableObject _disposableObject;

    //Some code

    public void Dispose()
    {
        if (_disposableObject != null)
        {
            _disposableObject.Dispose();
            _disposableObject = null;
        }
    }
}

I have performed some timings and found the following results:

valueClass with null check = 1361
nullClass with null check = 1173
valueClass with no null check = 1208
nullClass with no null check = 1148

As you can see without the null check it is slightly quicker but not enough for any significant optimisation. Also I performed these timings from the compiler in debug mode with optimisations turned off so they are not 100% accurate/relevant.

However, when I ran in release mode, with optimisations enabled outside of the compiler the results were so close together it was even more negligible to check or not.

And the test code:

using System;

namespace NullCheckTest
{
    class Program
    {
        const int iterations = 10000000;

        static void Main(string[] args)
        {
            MyClass valueClass = new MyClass() { Value = 10 };
            MyClass nullClass = null;

            Console.WriteLine($"valueClass with null check = {TestNullCheck(valueClass)}");
            Console.WriteLine($"nullClass with null check = {TestNullCheck(nullClass)}");

            Console.WriteLine($"valueClass with no null check = {TestNoNullCheck(valueClass)}");
            Console.WriteLine($"nullClass with no null check = {TestNoNullCheck(nullClass)}");

            Console.ReadLine();
        }

        static long TestNullCheck(MyClass myClass)
        {
            MyClass initial = myClass;

            System.Diagnostics.Stopwatch sw = new System.Diagnostics.Stopwatch();
            for (int i = 0; i < iterations; ++i)
            {
                sw.Start();

                if (myClass != null)
                {
                    myClass = null;
                }

                sw.Stop();

                myClass = initial;
            }

            return sw.ElapsedMilliseconds;
        }

        static long TestNoNullCheck(MyClass myClass)
        {
            MyClass initial = myClass;

            System.Diagnostics.Stopwatch sw = new System.Diagnostics.Stopwatch();
            for (int i = 0; i < iterations; ++i)
            {
                sw.Start();

                myClass = null;

                sw.Stop();

                myClass = initial;
            }

            return sw.ElapsedMilliseconds;
        }
    }

    public class MyClass
    {
        public int Value { get; set; }
    }
}
TheLethalCoder
  • 6,668
  • 6
  • 34
  • 69
  • 1
    Well, you are modifing a code of OP by adding something inside. That is not really an answer to his question. – Sinatr Feb 23 '17 at 13:30
  • @Sinatr Which is what my ending statement states. However, it is still an answer because it states a use case for this. – TheLethalCoder Feb 23 '17 at 13:31
  • 1
    And there are not that much cases for calling `Dispose` by hand. `using` is much more useful for this. – Patrick Hofman Feb 23 '17 at 13:34
  • @PatrickHofman Except in a disposable class with a field that needs disposing. That is a common use case for this in my opinion. – TheLethalCoder Feb 23 '17 at 13:35
  • 1
    If a class supports `IDisposable`, I will wrap it by `using` block when calling. No need to set to null. Also, if it is an instance of a class, just creates new instance again. Like `var mc = new MyClass();` – Tân Feb 23 '17 at 13:35
  • Note: You all seem to be getting to caught up in my provided example, it was just to show a use case for this i.e. perform some operations on the object. – TheLethalCoder Feb 23 '17 at 13:40
-2

To answer the question: No, there is no need.

However. As a general rule of thumb object variables and properties should never be null, as it forces you to always have to check the code if the value is being set somewhere.

What you're supposed to do, is use a custom class Maybe<> to indicate that an object variable or property might not be initialized.

Like so:

public class Maybe<T> where T : class
{
    public readonly T Value { get; }

    public Maybe(T value)
    {
        _Value = value;
    }

    public bool HasValue => _Value != null;
    public bool HasNoValue => _Value == null;

    public static operation Maybe<T>(T value) => new Maybe(value);
}

Then you can use it thus:

public class Foo
{
    public Maybe<Bar> Bar { get; set; }
    public int GetBarCount()
    {
        return Bar.HasValue ? Bar.Count : 0;
    }
}

public class Bar
{
    public int Count { get; set; }
}

The above code will work in all of the following examples:

Foo foo = new Foo();
foo.Bar = new Bar(); // Outputs 0 when GetBarCount is called
foo.Bar = new Bar { Count = 2 }; // Outputs 2 when GetBarCount is called
foo.Bar = new Maybe<Bar>(new Bar()); // Outputs 0 when GetBarCount is called
foo.Bar = new Maybe<Bar>(new Bar { Count = 2 }); // Outputs 2 when GetBarCount is called
foo.Bar = new Maybe<Bar>(null); // Outputs 0 when GetBarCount is called
foo.Bar = null;  // Outputs 0 when GetBarCount is called

Note! - When using Maybe<Bar> you explicitly mark Foo.Bar as not always having a value, and provides a strong signal of it. Not marking it with Maybe<Bar>, however, signals that it will always have a value. By using the Maybe<> wrapper in the right places, the code becomes much easier to work with, as you no longer have to do null checks for unwrapped object variables and properties.

TheLethalCoder
  • 6,668
  • 6
  • 34
  • 69
mireigi
  • 180
  • 7
  • 3
    Why obfuscate code like this? Also you are still checking for null, this wrapper class is confusing and pointless in my opinion. – TheLethalCoder Feb 23 '17 at 14:01
  • See the note I added at the bottom. It should explain it well enough. – mireigi Feb 23 '17 at 14:06
  • 1
    I understand the why you've done it, I just don't see the point for it. – TheLethalCoder Feb 23 '17 at 14:08
  • I first read it here, and have been using it ever since. It has helped improve the readability and useability of the code we write at work. It is of course up to the individual developer to use it or not. We've had great success with it, and so others might as well. http://enterprisecraftsmanship.com/2015/03/13/functional-c-non-nullable-reference-types/ – mireigi Feb 23 '17 at 14:13
  • 3
    What is the use of `return Bar.HasValue ? Bar.Count : 0;` over `return Bar?.Count ?? 0;`? This is creating massive overhead with no gain at all. – Patrick Hofman Feb 23 '17 at 14:27
  • The use lies in it being explicit that `Bar` might not have a value. To avoid your software crashing with a `NullReferenceException` when accessing `Bar.Count` when the instance of `Bar` is `null`, you will, without the `Maybe` wrapper, have to check the implementation of whatever code produced the value for `Bar`. Check the link I posted in the comment above this. It provides far more detail on the subject. – mireigi Feb 23 '17 at 14:38
  • 1
    Yuck. Yuck, yuck, yuck. – Craig Tullis Oct 16 '17 at 17:06