CODEWITHSUNDEEP
c#dependency-injection.net-coreconstructor-injectionnull-check
Joshua VdM
Joshua VdM

Reputation: 668

Is it necessary to check null values with constructor injection?

I'm using .NET Core constructor injection. In a code review from a colleague, he raised the question if I should check for null values on injected dependencies in controllers.

Since the framework is responsible for creating an instance of the service, it seems to me that it would take care of any errors and never have a null value dependency passed to a constructor. I don't have any factual evidence for this though, so I'd like to know if it's possible that a null check may be necessary.

For example, should I check if 'myService' is null in the following code? (Assuming the code is configured to use DI)

public class MyController
{
    private readonly IMyService _myService;

    public MyController(IMyService myService) 
    {
        _myService = myService;
    }
}

Upvotes: 15

Views: 12531

Answers (4)

KUTlime
KUTlime

Reputation: 8117

Either of these answers from Camilo Terevinto or gunr2171 will do any good for you.

Generally, we can discuss if trading one exception to another (NullReferenceException to ArgumentNullException) is a good deal or if it is a deal at all. Ask yourself if the latter brings any new information to you. My humble opinion in this particular case is No.

If any of your controller's dependency is null, you immediately know where is the problem.

One way or another, you know immediately what and where is the problem in your particular case. A null check in a constructor of a controller is just overhead. That is where the answer from Camilo Terevinto fails. You will never initiate it by yourself. The build-in DI service in .NET Core is designed to never inject a null into your constructors unless you instruct it to do so.

The answer from gunr2171 is talking about completely different aspect of DI. It is a valid answer but for a different question.

More than 80 % of DI use is a constructor injection. Most of these injection are related to MVC controllers and view models in Razor pages. MVC is a deprecated design pattern in ASP.NET Core 3.x and replaced by Razor Pages but the logic is pretty much same.

Any usable controller needs at least three dependencies:

  1. logger
  2. mapper
  3. repository

Thrust me here, you don't want to type:

_logger = logger ?? throw new ArgumentNullException(...)
_mapper = mapper ?? throw new ArgumentNullException(...)
_repository = repository ?? throw new ArgumentNullException(...)

to every single controller/view model you have.

There are definitely better ways how to do it. My personal recommendation is this answer in a duplicate question.

.NET 6 and beyond

There is a new method in .NET API ArgumentNullException.ThrowIfNull(someParameter).

This method can save you some typing if you insist on null checks.

Upvotes: 2

Heinzi
Heinzi

Reputation: 172388

I'll like to propose a different point of view. This...

private readonly IMyService _myService;

public MyController(IMyService myService) 
{
    _myService = myService ?? throw new ArgumentNullException(nameof(myService));
}

...is not only a run-time check for the execution environment, it's also a class invariant for future developers (cf. "Design by Contract").

Since _myService is readonly and every constructor contains a null check, a developer modifying/extending this class in the future will know for sure that the field _myService can never be null, even without knowing anything about how the class is used.

Another way to specify this invariant would be to add a comment to _myService...

// will always be provided by DI and, thus, will never be null
private readonly IMyService _myService;

...but adding a null-check to the constructor is the more idiomatic way to do that in C#.

Upvotes: -1

Camilo Terevinto
Camilo Terevinto

Reputation: 32063

Is it necessary to check null values with constructor injection?

It depends.

  • Is this internal code, used by you and (possibly) some teammates in a (fortunately) code-review environment?

Don't. It's not necessary. The framework doesn't allow this.

  • Is this code in a public library, used by multiple people or not actually following Dependency Injection?

Then do it. A manual instantation would result in a NullReferenceException somewhere and those are hard to track down.

That said, using something like this:

public MyController(IMyService myService) 
{
    if (myService == null)
    {
        throw new ArgumentNullException(nameof(myService));
    }

    _myService = myService;
}

Is an extremely cheap check and it's much easier to track down if someone passes null for some reason.

Even better, as @ScottFraley mentions, with newer C# versions, the above can be even more readable:

public MyController(IMyService myService) 
{
    _myService = myService ?? throw new ArgumentNullException(nameof(myService));
}

Upvotes: 20

gunr2171
gunr2171

Reputation: 17579

To get the obvious out of the way, there is no harm in doing a null check in your constructor.

There are two methods for obtaining DI services from the framework.

The first is GetService<T>(). This will return a null value if such a service has not been registered.

The second is GetRequiredService<T>(). This will throw an exception if the service can't be found.

Both of these methods throw exceptions if they can't fully instantiate the service you are asking for.

class Program
{
    static void Main(string[] args)
    {
        var services = new ServiceCollection()
            .AddTransient<IServiceB, ServiceB>()
            .BuildServiceProvider();

        var servB = services.GetService<IServiceB>();
    }
}

public interface IServiceA { }
public interface IServiceB { }

public class ServiceA : IServiceA { }
public class ServiceB : IServiceB { public ServiceB(IServiceA a) { } }

In this example, ServiceB requires an IServiceA, but A is not added to the dependency graph. The last method will throw an exception:

System.InvalidOperationException: 'Unable to resolve service for type 'DITests.IServiceA' while attempting to activate 'DITests.ServiceB'.'

If I instead did services.GetService<IServiceA>(), I'd get a null value.

You can see this for yourself by looking at the GitHub source code. When calling either method, it will eventually make it's way to the CreateConstructorCallSite method. This throws an exception if it's unable to resolve the dependencies of your type.

As for ASP.Net Core MVC, it uses GetRequiredService<>() for getting your controllers from the DI graph.

In conclusion, no, you do not need to perform null checks for DI objects in your constructor if you're using the pure Microsoft DI framework. As Camilo Terevinto said, the framework does not allow it.

As noted in your post, I have not seen a written Microsoft document that explicitly says you do not need to

If you are passing in an IServiceProvider as a constructor argument and you're resolving services from within the constructor, then you'll need to do null checks.

Upvotes: 5

Related Questions