Best way to handle a NULL

Question

At the top of my functions I'm trying the best way to handle a null coming into my procedures in C#. Which is the best way for checking and handling the null and why? I've added the complete code of what I'm using right now and Resharper is telling me to use Option #1. Normally I do what it says as I understand why it makes it more efficient. This time though I'm not sure so I must ask.

Option #1
if (sender == null) return;

// Code goes here

or

Option #2
if (sender != null)
{ 
     // Code goes here
}

Complete Code
        private void EmployeeMouseHoverToolTip(object sender, EventArgs e)
        {
            if (sender != null)
            {
                var sUserIdentifier = ((C1TextBox)sender).Text;
                var userIdentifier = Guid.Empty;
                if (Utilities.IsGuid(sUserIdentifier))
                {
                    userIdentifier = new Guid(sUserIdentifier);
                }

                var toolTipText = Utilities.UserIdentifierToName(userIdentifier);
                c1SuperTooltip.SetToolTip(sender as C1TextBox, toolTipText);
            }
        }

Answer 1

The best code is to disallow null (instead of what you’re doing). This isn’t always possible (sometimes it’s important to handle null in a meaningful way) – but in most of the cases it is.

Then all you need to do (in defensive coding) is to add a null check and throw an exception:

if (arg == null)
    throw new ArgumentNullException("arg");

Many (if not most) methods in the .NET framework and in good libraries do it that way.

Apart from that, the sender of an event should never be null and I’d say that a check for it is redundant. If null gets passed to this event, there’s something seriously wrong with your code.

The way you handle null (by silently swallowing it and doing nothing) may mask serious bugs in the application and is rarely, if ever, appropriate. Errors in the code should raise suspicious behaviour, not be swept under the carpet.

Answer 2

In your exact example here, go for the inverse of what you are doing, so if sender is null, go for an early exit. It reads better (IMO) and results in less nesting.

So Option #1 in this instance

Answer 3

I personally prefer the first option

if (sender == null) return;

It cuts down on nesting and increases readability.

Answer 4

If you're not going to handle a null sender, then I would go with the first option (and make sure it's the first line in the handler so it doesn't get hidden by other code).

If you think you may handle a null sender eventually, I would go with the second option since it offers you a better way to handle things as well as maintaining a single return point for the handler.

Answer 5

The performance impact is minimal, so I wouldn't even worry about that. Option 1 is better because it is more readable and less cloudy...

More readable because the conditional is not negated, and there isn't an unnecessary scope block.

Answer 6

Option 1 is being suggested by resharper, in my opinion, because it makes for easier to read code. You'll end up with:

• Less indenting
• Code that asserts its requirements and reacts to them right at the top of the method (if sender is null, I return, straight away)
• Code that's generally easier to maintain because it's clearer

As far as performance is concerned, there's probably little difference (though if it matters to you, measure it). There's nothing to stop the JIT compiler from re-writing one form to the other anyway, if they don't get output as identical MSIL by the C# compiler anyway.

Answer 7

I generally go with Option #1. I feel it's cleaner and it's meaning is more clear. Whoever's reading the code knows that if we've safely gotten past the null check and have exited then there's no chance of us messing around with a null value in sender later on.

Answer 8

Option #1 I guess would reduce Cyclomatic complexity. In option #2 , if there is another if conditions then it would come under the if clause and increase the complexity

Answer 9

I prefer

if (sender == null) return;

with it there are less nested operations in code and early exit if there is null.

Answer 10

Why not just pretend that a null reference never occurs, and don't catch the NullPointerException?

You get a stack trace, plenty of information, and it's handled as an exception.

Answer 11

Based on what ILDASM says, I would say option #2 is (slightly) more efficient:

Code:

class Program
{
    static void Main(string[] args)
    {
        Method1(null);
        Method2(null);
    }

    static void Method1(object sender)
    {
        if (sender == null)
            return;

        for (int x = 0; x < 100; x++)
        {
            Console.WriteLine(x.ToString());
        }
    }

    static void Method2(object sender)
    {
        if (sender != null)
        {
            for (int x = 0; x < 100; x++)
            {
                Console.WriteLine(x.ToString());
            }
        }
    }
}

ILDASM for Method1:

.method private hidebysig static void  Method1(object sender) cil managed
{
  // Code size       47 (0x2f)
  .maxstack  2
  .locals init ([0] int32 x,
           [1] bool CS$4$0000)
  IL_0000:  nop
  IL_0001:  ldarg.0
  IL_0002:  ldnull
  IL_0003:  ceq
  IL_0005:  ldc.i4.0
  IL_0006:  ceq
  IL_0008:  stloc.1
  IL_0009:  ldloc.1
  IL_000a:  brtrue.s   IL_000e
  IL_000c:  br.s       IL_002e
  IL_000e:  ldc.i4.0
  IL_000f:  stloc.0
  IL_0010:  br.s       IL_0025
  IL_0012:  nop
  IL_0013:  ldloca.s   x
  IL_0015:  call       instance string [mscorlib]System.Int32::ToString()
  IL_001a:  call       void [mscorlib]System.Console::WriteLine(string)
  IL_001f:  nop
  IL_0020:  nop
  IL_0021:  ldloc.0
  IL_0022:  ldc.i4.1
  IL_0023:  add
  IL_0024:  stloc.0
  IL_0025:  ldloc.0
  IL_0026:  ldc.i4.s   100
  IL_0028:  clt
  IL_002a:  stloc.1
  IL_002b:  ldloc.1
  IL_002c:  brtrue.s   IL_0012
  IL_002e:  ret
} // end of method Program::Method1

ILDASM for Method2:

.method private hidebysig static void  Method2(object sender) cil managed
{
  // Code size       44 (0x2c)
  .maxstack  2
  .locals init ([0] int32 x,
           [1] bool CS$4$0000)
  IL_0000:  nop
  IL_0001:  ldarg.0
  IL_0002:  ldnull
  IL_0003:  ceq
  IL_0005:  stloc.1
  IL_0006:  ldloc.1
  IL_0007:  brtrue.s   IL_002b
  IL_0009:  nop
  IL_000a:  ldc.i4.0
  IL_000b:  stloc.0
  IL_000c:  br.s       IL_0021
  IL_000e:  nop
  IL_000f:  ldloca.s   x
  IL_0011:  call       instance string [mscorlib]System.Int32::ToString()
  IL_0016:  call       void [mscorlib]System.Console::WriteLine(string)
  IL_001b:  nop
  IL_001c:  nop
  IL_001d:  ldloc.0
  IL_001e:  ldc.i4.1
  IL_001f:  add
  IL_0020:  stloc.0
  IL_0021:  ldloc.0
  IL_0022:  ldc.i4.s   100
  IL_0024:  clt
  IL_0026:  stloc.1
  IL_0027:  ldloc.1
  IL_0028:  brtrue.s   IL_000e
  IL_002a:  nop
  IL_002b:  ret
} // end of method Program::Method2

Answer 12

This is an event handler, it should only be called by controls in response to an event (never directly by your own code), so you shouldn't care about null checks or even type checks on the sender parameter (if you only attach this event handler to the same type of control). I'd do it simply like this:

private void EmployeeMouseHoverToolTip(object sender, EventArgs e) {  
  var txtBox = (C1TextBox)sender;
  var sUserIdentifier = txtBox.Text;
  var userIdentifier = Utilities.IsGuid(sUserIdentifier) ? 
    new Guid(sUserIdentifier) : 
    Guid.Empty;
  var toolTipText = Utilities.UserIdentifierToName(userIdentifier);
  c1SuperTooltip.SetToolTip(txtBox, toolTipText);
}

Actually, I'd go one step further and separate the logic to get the tooltip text from the logic to read and update the UI. Something like this:

private void EmployeeMouseHoverToolTip(object sender, EventArgs e) {  
  var txtBox = (C1TextBox)sender;
  var toolTipText = ResolveUpdatedTooltipText(txtBox.Text);
  c1SuperTooltip.SetToolTip(txtBox, toolTipText);
}

private string ResolveUpdatedTooltipText(string sUserIdentifier) {
  var userIdentifier = ResolveGuid(sUserIdentifier);
  return Utilities.UserIdentifierToName(userIdentifier);
}

private Guid ResolveGuid(string sUserIdentifier) {
  return Utilities.IsGuid(sUserIdentifier) ? 
    new Guid(sUserIdentifier) : 
    Guid.Empty;
}

Therefore, you shouldn't use any of the options you provided.

Answer 13

Don't check for it.

If you get nulls, you've added the handler to something you shouldn't have. And if some other bug causes it, you should be handling it with WinForms' global exception handler so the program doesn't bomb, logging it, and uploading the logs to your site whichever way you can to check for such errors.

Answer 14

Resharper likes option 1 as it is a pre-condition checker. When pre-conditions are not met, an early return is done.

Usually an early return is destructive for code-readability, but in this case it is very readable.

This way you can easily add extra pre-condition checks, like checking the contents of the EventArgs e, without having to rework the main function code.

Answer 15

Similar to @Konrad but I like the idea of adding the exception in the end if the code

if( sender != null )
{
    // Code goes here
    ....
} else
    throw new ArgumentNullExcpetion("sender");

So I vote for Option #2

Answer 16

A variation of option #1 that either returns right away, or throws an exception. The trick is knowing which method to use. My rule of thumb is that if it's part of a public interface, then throw an exception. If it's something you have control over deep down in your framework then just return immediately and handle the null reference at that level.

public void IHaveNoControlOverWhereThisMethodIsCalled(object arg)
{
    if(arg == null)
        throw new ArgumentNullException("arg");    
}

private void TheOnlyCallersOfThisMethodComeFromMe(object arg)
{
    //I should do all my public parameter checking upstream and throw errors
    //at the public entry point only.
    if(arg == null)
         return;

}

In the specific case of your event handler:

private void EmployeeMouseHoverToolTip(object sender, EventArgs e)
{
    var txtSender = sender as C1TextBox;
    if(txtSender == null) return;

    var sUserIdentifier = txtSender.Text;
    var userIdentifier = Guid.Empty;
    if (Utilities.IsGuid(sUserIdentifier))
    {
        userIdentifier = new Guid(sUserIdentifier);
    }

    var toolTipText = Utilities.UserIdentifierToName(userIdentifier);
    c1SuperTooltip.SetToolTip(sender as C1TextBox, toolTipText);
}

Answer 17

I do not handle null for private methods, as I always make sure no null value will be sent to my private methods. If something went wrong and null has been passed to private method, so the exception will be thrown from as it should be and I will know that I did something wrong. If you always check for null value for private method, you might be skipping some logical error at run-time and you will never know you got one in your code until it hits you in the production.