What is the best practice in case one argument is null?

Question

when validating methods' input, I used to check if the argument is null, and if so I throw an ArgumentNullException. I do this for each and every argument in the list so I end up with code like this:

 public User CreateUser(string userName, string password, 
                            string Email, string emailAlerts, 
                            string channelDescription)
    {

        if (string.IsNullOrEmpty(userName))
            throw new ArgumentNullException("Username can't be null");

        if (string.IsNullOrEmpty(Email))
            throw new ArgumentNullException("Email can't be null");
       //etc, etc, etc
    }

Is this OK? Why should I do this? Would it be ok if I simply group all the checks and return a null value instead of throwing the exception? What is the best practice to address this situation?

PS: I want to change this, because with long methods, it gets very tedious to do so.
Ideas?

Answer 1

Make an ArgChecker class with something like this

  ArgChecker.ThrowOnStringNullOrEmpty(userName, "Username");

where ThrowOnStringNullOrEmpty is

  public static void ThrowOnStringNullOrEmpty(string arg, string name)
  {
      if (string.IsNullOrEmpty(arg))
        throw new ArgumentNullException(name + " can't be null");
  }

You could also try to process a list of arguments using a params arg, like:

  public static void ThrowOnAnyStringNullOrEmpty(params string[] argAndNames)
  {
       for (int i = 0; i < argAndName.Length; i+=2) {
          ThrowOnStringNullOrEmpty(argAndNames[i], argAndNames[i+1]);
       }
  }

and call like this

  ArgChecker.ThrowOnAnyStringNullOrEmpty(userName, "Username", Email, "email");

Answer 2

An approach which I use and I may have picked up from the NHibernate source is to create a static class Guard, used as follows:

public void Foo(object arg1, string arg2, int arg3)
{
    Guard.ArgumentNotNull(arg1, "arg1");
    Guard.ArgumentNotNullOrEmpty(arg2, "arg2");
    Guard.ArgumentGreaterThan(arg3, "arg3", 0);
    //etc.
}

public static class Guard
{
    public static void ArgumentNotNull(object argument, string parameterName)
    {
        if (parameterName == null)
            throw new ArgumentNullException("parameterName");

        if (argument == null)
            throw new ArgumentNullException(parameterName);
    }
    //etc.
}

This cuts down a lot of the chaff at the beginning of methods and it performs well.

Answer 3

You should think about the method, what it needs to do and with what data. If null values represent actual failure conditions, use exceptions. If null values are acceptable, accept them.

Think about the principles from design by contract, specifically what the preconditions to your function are, and standardize a way to enforce them (which Matt and Lou both suggest in their answers so I don't need to go into detail).

Another important thing to consider is the size of your method signatures. If you have a lot of parameters for your methods, this probably means you have bad abstractions. You can cut down on the number of parameter checks you have to make if you group parameters together in collection objects and use those objects as parameters. You can move the parameter checking to those objects instead of having to check them in every function that uses them.

So instead of passing ten related parameters to every function, figure out the few that are used in every function and package them up in an object, and include in that object methods to validate the parameters. This has the added advantage of being easy to change should the rules regarding one parameter need to be updated.

Answer 4

A small improvement to Lou's answer would be to use a hashtable instead, it means it checks objects aswell as just strings. Also just nicer to populate and handle in the method:

public static class ParameterChecker
{
    public static void CheckForNull(Hashtable parameters)
    {
        foreach (DictionaryEntry param in parameters)
        {
            if (param.Value == null || string.IsNullOrEmpty(param.Value as string))
            {
                throw new ArgumentNullException(param.Key.ToString());
            }
        }
    }
}

As you would use like:

public User CreateUser(string userName, string password, string Email, string emailAlerts, string channelDescription)    
{
    var parameters = new Hashtable();
    parameters.Add("Username", userName);
    parameters.Add("Password", password);
    parameters.Add("EmailAlerts", emailAlerts);
    parameters.Add("ChannelDescription", channelDescription);
    ParameterChecker.CheckForNull(parameters);

    // etc etc
}

Answer 5

My first advice to you is get ReSharper. It will tell you when there is a problem of possible null values, and when there is no need to check for them, and with the click of a mouse will add checking. Having said that...

You don't usually have to check for null strings and integers, unless you are receiving information from some legacy source.

Strings can be checked with string.IsNullOrEmpty()...

If you still decide that you want to check each and every parameter, you can either use the Command design pattern, and reflection, but your code will be unnecessarily clunky, or use the following and call it for each method: private myType myMethod(string param1, int param2, byte[] param3) { CheckParameters("myMethod", {param1, param2, param3}); // rest of code...

And in your utility class put this:

///<summary>Validates method parameters</summary>
///... rest of documentation
public void CheckParameters(string methodName, List<Object> parameterValues) 
{
    if ( string.IsNullOrEmpty(methodName) )
       throw new ArgumentException("Fire the programmer! Missing method name", "methodName"));

    Type t = typeof(MyClass);
    MethodInfo method = t.GetMethod(methodName);
    if ( method == null )
       throw new ArgumentException("Fire the programmer! Wrong method name", "methodName"));
    List<ParameterInfo> params = method.GetParameters();
    if ( params == null || params.Count != parameterValues.Count )
       throw new ArgumentException("Fire the programmer! Wrong list of parameters. Should have " + params.Count + " parameters", "parameterValues"));

    for (int i = 0; i < params.Count; i++ )
    {
            ParamInfo param = params[i];
            if ( param.Type != typeof(parameterValues[i]) )
                throw new ArgumentException("Fire the programmer! Wrong order of parameters. Error in param " + param.Name, "parameterValues"));
            if ( parameterValues[i] == null )
                throw new ArgumentException(param.Name + " cannot be null");
    }
} // enjoy

Answer 6

I would stick with your original approach, except for just passing in the parameter name. The reason is that once you start writing those helper procedures it becomes an issue when everyone starts using different conventions for how they write the helper procedures. When someone looks over your code, they now have to check to make sure you've written the helper procedure correctly when debugging your code.

Keep checking each argument separately, though youor fingers grow weary from typing Grasshopper :) Your followers will bless you when they get the unexpected ArgumentException and are saved from a debugging run just to determine which argument failed.

Answer 7

And for the C# 3.0 developers amongst us a great way to encapsulate this null checking is inside an extension method.

public void Foo(string arg1, int arg2)
{
  arg1.ThrowOnNull();
  arg2.ThrowOnNull();
}

public static class extensions
{
    public static void ThrowOnNull<T>(this T argument) where T : class
    {
        if(argument == null) throw new ArgumentNullException();
    } 
}

And if you wanted you could always overload that to take an argument name.