Better way of checking null in dependency injection

Question

When using dependency injection through a constructor I always need to check for nulls before passing the instance to an internal property. e.g.

public UserManager(User user, IStateManager stateManager)
{
    if(user == null) throw new ArgumentNullException;
    if(statemanager == null) throw new ArgumentNullException("stateManager");

    _user = user;
    _stateManager = statemanager;
} 

repeating this pattern on every controller / class seems repetitive. Is there a better way to handle this? btw different controllers will have different constructor initialisers. I am using Simple Injector for my DI.

Answer 1

It's repetitive code, but it will hardly ever be a problem, because can this cause sweeping changed through your code base? Will you ever need to change many of those checks? Hardly. Take a look at this blog post that goes in more details into this.

To be honest, when it comes to my injection constructors, I almost never add those null checks anymore, because I know my DI container will not inject null references into my constructors when auto-wiring those types. This saves me from writing all these null checks at all.

Some people might argue that I now write my code with my DI container in mind, but I would argue against that. I'm just writing the minimal amount of code required that solves my problems. Adding those null checks doesn't help me in my case.

But do note that in case I'm writing code for a reusable library, I absolutely do write those null checks, because I have no idea who is calling that code. For constructors that are not used as injection constructors (messages, entities, value types, DTOs) I actually DO add those checks. But here are some ideas how to make this a little bit nicer:

You can add a nice helper method like this:

public UserManager(User user, IStateManager stateManager)
{
    Requires.IsNotNull(user, "user");
    Requires.IsNotNull(statemanager, "statemanager");

    _user = user;
    _stateManager = statemanager;
}

This however, doesn't really help with reducing duplicate code, although it does reduce the actual size of machine code that is generated (but this hardly ever an issue). So, you could make this method return a value like this:

public UserManager(User user, IStateManager stateManager)
{
    _user = Requires.IsNotNull(user, "user");
    _stateManager = Requires.IsNotNull(statemanager, "statemanager");
}

Or... using C# 6.0:

public UserManager(User user, IStateManager stateManager)
{
    _user = Requires.IsNotNull(user, nameof(user));
    _stateManager = Requires.IsNotNull(statemanager, nameof(statemanager));
}

You can implement this method as follows:

public static class Requires {
    public static T IsNotNull<T>(T instance, string paramName) where T : class {
        // Use ReferenceEquals in case T overrides equals.
        if (object.ReferenceEquals(null, instance)) {
            // Call a method that throws instead of throwing directly. This allows
            // this IsNotNull method to be inlined.
            ThrowArgumentNullException(paramName);
        }

        return instance;
    }

    private static void ThrowArgumentNullException(paramName) {
        throw new ArgumentNullException(paramName);
    }
}

With C# 8 non-nullable reference types, reference types can be made non-nullable by default:

public UserManager(User user, IStateManager stateManager)
{
    _user = user;
    _stateManager = statemanager;
}

Note that this is only causes a compile-time enforcement, not a runtime enforcement. So no exceptions are thrown.

This might change with C# 9. There is a proposal for added the runtime checks using the ! symbol:

public UserManager(User user!, IStateManager stateManager!)
{
    _user = user;
    _stateManager = statemanager;
}

Answer 2

I use a static method, .ThrowIfNull, which will throw an ArgumentNullException with the correct argument name, if null.

public MyClass 
{
  public MyClass(DependencyType1 firstDependency, DependencyType2 secondDependency)
  {
     Arguments.ThrowIfNull(firstDependency, secondDependency);

     _firstDependency = firstDependency;
     _secondDependency = secondDependency;
  }
}

public static class Arguments
    {
        public static void ThrowIfNull(params object[] args)
        {
            for (var i = 0; i < args.Length; i++)
            {
                if (args[i] != null
                    && args[i].GetType() == typeof(ArgumentAction)
                    && (ArgumentAction)args[i] == ArgumentAction.Skip) continue;
                if (args[i] == null) throw GetArgumentNullException(i);
            }
        }

        private static ArgumentNullException GetArgumentNullException(int argIndex)
        {
            var frame = new StackFrame(2, false);
            var method = frame.GetMethod();
            var args = method.GetParameters();
            var name = args[argIndex].Name;
            return new ArgumentNullException(name);
        }

        public enum ArgumentAction
        {
            Undefined = 0,
            Skip
        }
    }

Answer 3

In fact such checks produce 0 business value, but create extra noisiness in your code. A fellow developer will be confused and will have to spend extra effort trying to understand why you have all these checks. Normally IoC container will throw an error when the dependency could not be resolved. Even if your container did not throw, you'll have null reference exception later during execution. Better add Debug.Assert statement, which is sort of indication that you are not sure what exactly is happening here, but you've got null sometimes