How not to do Dependency Injection, in NerdDinner

Checking out the NerdDinner code the other day, I found a common Dependency Injection anti-pattern.  One of the core concepts of DI is that components are not responsible for locating their own dependencies.  The code went part of the way to full-on DI, but not quite far enough.  Here’s the offending code:

public class SearchController : Controller {

IDinnerRepository dinnerRepository;

//
// Dependency Injection enabled constructors

public SearchController()
    : this(new DinnerRepository()) {
}

public SearchController(IDinnerRepository repository) {
    dinnerRepository = repository;
}

The second constructor is correct – the SearchController’s dependencies are passed in through the constructor.  This is because the IDinnerRepository is a required, or primal dependency.  SearchController depends on IDinnerRepository to function properly, and won’t work without it.  But on the first constructor, we violate DI by having the SearchController create a concrete DinnerRepository!  We’re now back to concrete, opaque dependencies.  We have a small benefit of easier testability, but we still force our controller to locate its own dependencies.

So why is this a Bad Thing?

For one, it’s confusing to have two constructors.  Why go through all the trouble of creating the IDinnerRepository interface if I’m just going to depend directly on an implementer?  Now what if DinnerRepository now needs some dependency?  What if it now needs some ILogger, security or policy dependency?  Do I now need to go fix all of my calls to “new”?

And that’s the whole point of Dependency Injection, and the Dependency Inversion Principle.  I only know about my first level dependencies, and abstractions on top of that.  If we check out DinnerRepository, we see another issue:

public class DinnerRepository : NerdDinner.Models.IDinnerRepository {

    NerdDinnerDataContext db = new NerdDinnerDataContext();

A private, opaque dependency to NerdDinnerDataContext.  If we wanted to make that opaque dependency explicit, we’d have to fix our SearchController (and all the other controllers as well).  It’s these kinds of ripple effects that prevent refactoring and improvement.

###

Fixing it

But we can fix this, quite easily.  From MvcContrib, we can grab any one of the ControllerFactory implementations for the IoC container of our choice.  For me, that’s StructureMap.  First, we’ll need to configure StructureMap to wire up all of our dependencies.  The preferred way to do so is to create a Registry:

public class NerdDinnerRegistry : Registry
{
    public NerdDinnerRegistry()
    {
        Scan(scanner =>
        {
            scanner.TheCallingAssembly();
            scanner.WithDefaultConventions();
        });

        ForRequestedType<NerdDinnerDataContext>()
            .CacheBy(InstanceScope.HttpContext)
            .TheDefault.Is.ConstructedBy(() => new NerdDinnerDataContext());
    }
}

In the first part, we just tell StructureMap to scan the calling assembly with default conventions.  This will wire up IDinnerRepository to DinnerRepository.  Probably 99% of our dependencies will be taken care of in that one call.  Next, we need to wire up the NerdDinnerDataContext (for reasons we’ll see soon).  Since that class has multiple constructors, StructureMap likes to use the greediest dependency, with the most arguments.  I don’t want that, so I override it to use the no-arg constructor.  Finally, I cache it by HttpContext, though I could probably go Singleton if it’s expensive to instantiate.

Next, I need a wrapper class to initialize StructureMap:

public static class IoCContainer
{
    public static void Configure()
    {
        ObjectFactory.Initialize(init =>
        {
            init.AddRegistry<NerdDinnerRegistry>();
        });
    }
}

Initialize only needs to get called once per AppDomain, and all I need to do is add the NerdDinnerRegistry I created earlier.  I could wire up everything here, but again, the preferred configuration method is through registries.  The next piece is to wire up ASP.NET MVC to both call our configuration method and use the StructureMapControllerFactory:

void Application_Start()
{
    RegisterRoutes(RouteTable.Routes);

    IoCContainer.Configure();
    ControllerBuilder.Current.SetControllerFactory(new StructureMapControllerFactory());

    ViewEngines.Engines.Clear();
    ViewEngines.Engines.Add(new MobileCapableWebFormViewEngine());
}

I put all this in with the other MVC configuration in the Global.asax Application_Start method.  One final piece is to remove the extra constructors on our controller:

public class DinnersController : Controller {

    IDinnerRepository dinnerRepository;

    //
    // Dependency Injection enabled constructors

    public DinnersController(IDinnerRepository repository) {
        dinnerRepository = repository;
    }

And make our DinnerRepository expose its dependencies explicitly:

public class DinnerRepository : NerdDinner.Models.IDinnerRepository {
    
    private readonly NerdDinnerDataContext db;

    public DinnerRepository(NerdDinnerDataContext db)
    {
        this.db = db;
    }

Note that there are no no-arg constructors to be found!  When StructureMap creates a DinnersController, it will look to resolve the IDinnerRepository dependency.  That’s a DinnerRepository, which in turn needs the NerdDinnerDataContext.  But that’s all hidden from us, we never need to wire up an entire graph, as we would if we stuck to the “new” operator.  Just to make sure everything works, I can navigate to view upcoming dinners:

##

Wrapping it up

In the original NerdDinner code, dependency inversion was only partially implemented as the original controllers still contained a constructor that called a constructor of a concrete class.  To fix this, we added StructureMap to the mix, configuring ASP.NET MVC to use StructureMap to create controllers instead of the default controller factory.  Finally, we pushed the dependency inversion principle all the way down to our repository, removing the opaque dependencies where we found them.  When we finished, all of our classes exposed their dependencies explicitly through their constructor.  No one class knew more than one level of depth, and our controllers now properly depended exclusively on the IDinnerRepository abstraction.

In the future, we can add things like logging, policies and the like to custom IDinnerRepository implementations, without needing to change any of our controllers.  Once we introduce inversion of control to our application, we open a lot of doors for functionality and simplicity, but only going halfway won’t give us the full benefit.