Where to check if an object is null or not?

Question

Where do you check if an object that you are passing to a method is null or not?

Should an object need to be tested before calling a method? or within the method that is using the argument?

public class Program
{
    public static void Main(string[] args)
    {
        // Check if person is null here? or within PrintAge?

        PrintAge(new Person { Age = 1 });
    }

    private static void PrintAge(Person person)
    {
        // check if person is null here?

        Console.WriteLine("Age = {0}", person.Age);
    }
}

public class Person
{
    public int Age { get; set; }
}

Having a "null" check in both classes seem to be too much redundant code.

[EDIT]: What would be an dis/advantage of checking for null within a caller or a callee?

[EDIT2]: I just ran into Defensive Programming and it seems like it advocates checking null within a callee. I wonder if this is a widely accepted practice.

Answer 1

If you design a library, there will be methods exposed to the outer world. You should check the incoming data in this methods. No checks are required in methods that you do not expose, because only your code calls them and its logic should handle all cases you accepted in the exposed method called.

                    --------------------------
                   |                          |
                   |         Library          |
                   |                          |
 -------        ---------        ----------   |
|       |      |         |      |          |  |
| Outer |      | Library |      | Library  |  |
|       | ===> | Entry   | ===> | Backend/ |  |
| World |      | Method  |      | Helpers  |  |
|       |      |         |      |          |  |
 -------        ---------        ----------   |
                   |                          |
                   |                          |
                    --------------------------

If you have accepted the supplied data in the entry method, you should perform the requested action and return the expected result, that is handle all remaining cases.

UPDATE

To clarify the situation inside the library. There might be null checks, but only because of the logic, not because of parameter validation. There are two possibilities for the location of null checks inside the library. The first one if the called method knows how to handle null values.

private CallingMethod()
{
   CalledMethod(someData);
}

private CalledMethod(Object parameter)
{
   if (parameter == null)
   {
      // Do something
   }
   else
   {
      // Do something else
   }
}

And the second situation if you call a method that cannot handle null values.

private CallingMethod()
{
   if (someData == null)
   {
      // Do the work myself or call another method
   }
   else
   {
      CalledMethod(someData);
   }
}

private CalledMethod(Object parameter)
{
   // Do something
}

The whole idea is to reject cases you cannot handle immediately and handle all remaining cases properly. If the input is not valid you throw an exception. This forces the library caller to supply only valid values and does not allow the caller to continue execution with meaningless return values (besides the caller shallows the exception an continues).

Answer 2

I prefer null checks inside methods for two reasons.

1. I think functions should be 'complete', ie handle null values/'edge cases' and not rely on callers. This is for two reasons,

   • someone calling the method later might forget to add null checks
   • it's easier to test the method with edge cases in unit tests.

2. having null checks inside the method reduces overall number of null checks inside the code, which usually means more readable code

Answer 3

You can design a method to work with valid objects only.

That means you are expect to receive valid objects ( not null in your case ).
That means you don't know how to react and what to do with invalid objects:

• returning silently from the function is not a solution;
  
• throwing an exception will mean you move responsibility to the upper methods where they can check the value already before passing to you.
  

So if your method don't know exactly how to handle invalid object and the method won't follow additional logic in the invalid case you should put

Debug.Assert( Person );

at the PrintAge begin and this will force you to make checks upper by call stack.

The lower function in hierarchy is the less checks it should do. The following is disadvantages of doing checks in the functions that do the work.

• Function that is doing actual work has to be as clear as possible without mass of ifs
• Function will be called more then many times
• Such function can call such functions and they can call such functions again. Each of them will perform the same validation

Answer 4

There is only one occasion that a constructor can return null [new() on a Nullable<T>] - so the calling code doesn't need to check.

The callee probably should check; throwing an ArgumentNullException if it was null. In .NET 4.0 this will be better served by code contracts. But not yet ;-p

Answer 5

As I understand your question it is more general than illustrated by your example. My preferences are as follows:

• All publicly accessible methods must check for NULL input and throw exceptions as appropriate. So if you're building a framework for others to use, code defensively.
• Private methods may omit the NULL check if you know that this is done elsewhere or that arguments will never be NULL, but in general I prefer the explicit ArgumentNullException to NullRefereceException.

Brad Abrams has some more input here: http://blogs.msdn.com/brada/archive/2004/07/11/180315.aspx

Answer 6

PrintAge should be a method on Person, not a static taking a Person as parameter. No check needed.

Checking for null values makes the code unnecessarily complex. Structure your code so as to limit (or eliminate) the occasions where null is a possible value, and you'll have much fewer checks to write.

Answer 7

Definitely check in PrintAge, it's a right place to check. It can be redundant but won't hurt anyone unless you execute it 1000 times per second. (Depending on the check throw an exception or fix it if you can)

Other check is depend on your actual flow, in this example you don't have a flow so I can't comment on that bit. But generally consider your parameters as tainted.

Answer 8

You've got nothing to check in Main - you're using the new operator which never returns null (except for Nullable<T>).

It would be entirely reasonable to check in PrintAge, particularly if it were made public. (For private APIs it's less important to do argument checking, but it can still be very useful.)

if (person == null)
{
    throw new ArgumentNullException("person");
}

These days in C# 3.0 I usually use an extension method for this.

Answer 9

Redundant code isn't the most elegant but its safe.

This depends on who your intended user is, if its you then your in control of how everything is used and the checks are only necessary if your unsure of what the state of your variables will be.

If your making this for someone else to use then null checks are probably a good idea. Even if you just throw a NullPointerException its better to fast fail.

Answer 10

I normally let my null checks be controlled by my expectations; if I expect something to be null or am unsure of it, I add a check. Otherwise I don't. Nulllpointer exceptions are among the easiest problems to track, so excessive sprinkling of checks bloats code. In the specific example I'd check nothing, because it's intutitive it's not null.

Answer 11

What would you want to do, if instance is null?

I think it depends on the API you provide & define contract (the way .net framework classes do). Having said that, you need not do a check for null (in main), if the method defines what is expected outcome in case of null reference passed to it.

Answer 12

I would say that checking it n PrintAge seemed to make more sense as that is fulfiling the contract for the routine. You could, of course, replace the null checks with Debug.Assert() code to check at test time, but not at release time.

Answer 13

You mean checking in both methods? I'd check in PrintAge for sure and if it makes sense within Main as well. I don't think there is a definite answer in general. It depends :-)