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:

image

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.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in ASP.NET MVC, Refactoring, StructureMap. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://neilmosafi.blogspot.com Neil Mosafi

    It’s known as “poor man’s dependency injection”.

  • http://jonkruger.com/blog Jon Kruger

    They probably did it that way because they couldn’t force anyone to use a specific DI container. I see other problems in MVC samples where they probably wanted to use a DI container but couldn’t, so they end up with half-baked dependency injection.

  • http://eric.polerecky.com Eric Polerecky

    Nice article; you show how to jump into DI with little overhead. For me; ND is a good sample of a simple way to starting to think about loose coupling without additional software.

    My largest client is a very large county government; the amount of hoops I have to jump thru to use any new software is mind blowing. ND offers me a good sample to show DI…well more just loos coupling…with zero overhead (additional software)

  • http://twitter.com/smiller Shawn Miller

    Thanks for demonstrating this implementation. To be fair, the Professional ASP.NET MVC 1.0 book makes this point:

    “ASP.NET MVC exposes extensibility APIs that enable developers to participate in the resolution and instantiation of controllers, and that enables Dependency Injection/IoC frameworks to be cleanly integrated within this process. Using a DI/IOC framework would also enable us to remove the default constructor from our DinnersController – which would completely remove the coupling between it and the DinnerRepositorys.

    We won’t be using a dependency injection/IOC framework with our NerdDinner applicaiton. But it is something we could consider for the future if the NerdDinner code-base and capabilities grew.”

  • http://scottmuc.com/ Scott Muc

    I personally don’t see that as a terrible thing. If you know what the negatives are, it’s a great way to develop a SMALL project without introducing a container.

    The code is still testable because the constructor still allows the injection of fakes.

    I believe this style of DI has a name. It’s called “Poor Mans Dependency Injection”, and I personally believe it’s fine for personal projects. Whether or not it’s a good thing for NerdDinner to be doing is another question. I’m sure they implemented DI that way knowing full well the negatives of that implementation.

  • http://kyle.baley.org Kyle Baley

    Yeah, I wouldn’t fault anyone for using poor-man’s dependency injection. At least not without looking at the rest of the code. It’s a nice first step to get your feet wet with dependency injection, yesno? Then you can move on to maybe rolling your own DI container, then switching to a third-party one. That’s just based on how I was taught, I guess.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Kyle

    No, in their example, I think it’s quite reasonable, given the size of the app. Since it’s so easy to do real DI, I don’t see the pain of doing the real thing.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Neil

    Thanks, I forgot that term!

  • http://www.lostechies.com/members/chadmyers/default.aspx chadmyers

    If they don’t want to force you to use DI/IoC, they should use their own default one (maybe embed the source from one of the micro-IoC frameworks into your code as long as its all open source).

    Self-servicing dependencies via the c’tor is almost always wrong. I’d say this is one of the ‘definitely wrong’ cases.

  • http://mynerditorium.blogspot.com Daniel Auger

    Just wondering – is a container going to be used throughout the Asp.NET MVC in Action book?

  • http://blog.dashpoint.com Rod Paddock

    Hey dood! This blog post was EXACTLY what I happened to be looking for in some development I am doing now. Good job!

  • http://chaliy.pip.verisignlabs.com/ Mike Chaliy

    We moved to the poor-man’s dependency injection from proper DI. And now I do not think that this is anti pattern(as I did before).

    Primmary reasons are:
    1) api usage is much more more netural…
    2) consumer control creation(or access) that means that u can be lazy, an object that you depends on are also could be lazy. you can force instantiation by factory methods.. thats still much more netural.
    3) you do not need materialize whole graph of dependecies… means instantiation of the controller could result to instantiation of the dep1 and dep2, but could just dep1.
    4) there is no configs (implict or explict)… thats cool!
    5) we are not forced to use procedure style code (services are plain bags of procedures), we can do proper OO. Composition now much more netural.
    6) Public contrustors are now constructors, they are not overloaded with all dependecies.
    7) YAGNI

    We are looking on possibilties to move our server code to this paradigm.

  • http://thomaseyde.blogspot.com Thomas Eyde

    I agree with most of your points, but not on exposing the DataContext:

    The DinnerRepository is the implementation, the use of Linq 2 SQL is an implementation detail, which does not belong outside of this class. If we should move to a different ORM, the whole class needs a rewrite, anyway.

    The other thing is that the DataContext is a Unit of Work, and the last thing we want is to have that as a singelton. Just figuring out when to commit is painful enough. Keeping it in the HttpContext doesn’t make it easier to control.

  • http://rafanoronha.net/ Rafael Noronha

    I think common scenarios are not hugely requiring DI containers usage.
    I’m pretty sure that this partial implementation is not that bad when used in high-level components like Controllers.

    Keeping things simple tends to be a good thing.

    In scenarios that really require dynamic binding a DI container would be really cool.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Mike

    Funny, DIP solves all the problems you just laid out. Poor Man’s DI hides or worsens them. The “new” operator is strong coupling to a specific implementation. Poor man’s DI is actually _more_ work, as I now have to wire myself up explicitly. If you don’t like loose coupling, by all means, go the route you’ve laid out.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Thomas

    I’m not that familiar with L2S, but if I have two repositories in the same request, do I not want them to have the same DataContext?

    DIP is not all about swappability. It’s about exposing your dependencies and removing responsibility for locating them.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Rafael

    Suppose my controller depends on other higher-level application services, do I have to know everything about them too? Poor Man’s DI at the outermost layer of an application means that every single other dependency used also needs Poor Man’s DI. I don’t see Poor Man’s DI as simple. It requires extra work, all the way down. How is that simple?

  • http://thomaseyde.blogspot.com Thomas Eyde

    @bogardj

    You probably want the same datacontext for both repositories if one call to SubmitChanges() applies to both repositories.

    SubmitChanges() also represent your transaction boundary if you are not using TransactionScope.

    If your repositories should operate in separate transaction boundaries, you probably need two DataContexts.

    A final note: There is a conflict here between exposing dependencies and exposing implementation details. In this case I think it’s reasonable to hide the details.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Thomas

    I see a difference between the work being done vs. what the work is being done to/with. The implementation details here are the queries, not the query provider.

  • http://rafanoronha.net/ Rafael Noronha

    Jimmy,

    When the dependency implementation will always be the same, I don’t see Poor Man’s DI as a real issue.

    The great DI profit comes from contract dependencies.
    Implementation knowledge at the constructor level is not that bad when swappability is not wished.

    BTW, container configuration also requires extra work, don’t ?

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Rafael

    The DIP is not about swappability, that’s just a side-effect. It’s about 1) depending on abstractions, 2) low coupling and 3) removing responsibility for locating dependencies.

    It’s far, far more work to do poor man’s DI than regular DI. How many no-arg ctor’s do I need to write, versus a few lines of StructureMap configuration code?

  • http://devordinarie.wordpress.com Afif

    Hey Jimmy,
    One small request., if I have some client component that needs to talk to a DinnersController, how would it get to it? It won’t new it up., and lets say its a stateless component so it can’t have a c’tor dependency .

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Afif

    For one, it would likely depend on IDinnersController, as you wouldn’t depend on the concrete type. Second, at that point, you’re looking at doing service location. That’s actually what ObjectFactory.GetInstance (or equivalent) is, service location. Going to a well-known location to get instances.

    When using DI and an IoC container, the idea is to minimize the calls to the service locator. Eventually you _have_ to use it, but the fewer, the better.

  • http://thomaseyde.blogspot.com Thomas Eyde

    @bogardj

    Regarding the DataContext, I still think you are mistaken. The DataContext is very concrete, so locating it is not a problem. We don’t gain anything there.

    And in this particular case, if a dependency container creates it, how do we control the transaction scope? When should we call SubmitChanges?

    NerdDinner, if I remember correctly, use the Linq 2 SQL classes directly as domain models. So un this case, I guess you can say the queries are the implementation details.

    But I think you are stretching it. Linq 2 SQL doesn’t support all Linq queries, and there are projects that use L2S classes as DTO’s. In those cases the L2S classes never escape the repository and the DataContext has no meaning outside of the repository.

  • http://mikehadlow.blogspot.com/ Mike Hadlow

    @Thomas

    We use L2S with DI in Suteki Shop
    http://code.google.com/p/sutekishop/
    We have an IDataContextProvider implementation that we mark as ‘per web request’. So any repository instantiated in during that request would share the same dataContext. We control the unit-of-work with a UnitOfWork action filter which effectively provides a UOW-per-action pattern. You should not have a SubmitChanges method on your repositories.

    Have a look at the source, any comments would be welcome :)

  • http://brendan.enrick.com/ Brendan Enrick

    You’re very right that “the poor man’s dependency injection” is bad, but in some instances I think it is the right track to take. When making modifications to existing code, I prefer using it temporarily to get a good harness in place testing the code as this method has very little disruption to the existing code and is a quick easy improvement. I agree that it should be eliminated eventually, but I consider it a step along the path to improving dependency-rich code. It is also a great way to teach people new to DI the basic concept, because they can see the injection without having to make many modifications. Then once they understand that you show them how to remove that last dependency.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Brendan

    Yeah, I regret not making that point. Poor man’s DI is a valid dependency breaking technique for working with legacy code, and I should have pointed that out.

  • http://www.hanselman.com Scott Hanselman

    Thanks for doing this. I’ll write a follow up tonight. This, along with the AutoMapper example from Matt, would be a great addition to a v2 release of ND.

    Great post, good tone, good analysis, excellent comments.

  • http://www.grumpydev.com Steve Robbins

    I have no problem with “poor man’s DI”, as a container can be overkill for small projects, but I’d much rather see a static (or statics) on the class that return the dependency cast as an interface, instead of the default ctor approach. At least that way you still have the constructor injection, and inversion of control, you just don’t have the container doing the resolution for you.

    With the default constructor route I’d also be worried that I’d miss an instantiation if I decided at a later date to introduce a container, which might make for “fun” bugs :-)

  • http://www.aaronlerch.com/blog/ Aaron Lerch

    I just had a whole comment written up and I realized that most of it was moot. :) It seems that you’re using the terms Dependency Injection and the Dependency Inversion Principle interchangably in your post, when I don’t see the two as being interchangable. In your DIP post from back yonder you even said:

    “The Dependency Inversion Principle, or DIP, is often used interchangeably with Dependency Injection and Inversion of Control. However, following DIP does not mean we must automatically use a IoC container like Spring.NET, Windsor or StructureMap. IoC containers are tools to assist in applications adhering to DIP, but we can follow DIP without using IoC containers.”

    The NerdDinner source (at least in the SearchController example above) followed the Dependency Inversion Principle but didn’t implement Dependency Injection. Do I wish they had? Heck yeah. But it doesn’t seem fair to say this: “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.”
    The entire functional part of the class really does depend on an abstraction, which is the core of DIP. The fact that a “convenience” overload of the constructor contains the concrete coupling is a DRY issue, or a maintenance issue, but not a DIP issue.
    But anyway, besides it being a little confusing by mixing some terms (IMHO) I thought it was a good post. :)

  • http://rafanoronha.net/ Rafael Noronha

    @Aaron made a good point here.

    Poor Man’s DI really seems to me a DRY, not a DIP issue.
    Thinking in DRY, I agree a container can do a nice job.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Aaron

    I still don’t think it’s _real_ DIP if I do the overloaded no-arg ctor. I’m still partially depending on a concrete class, as if that class’s ctor changes, it affects my controller.

    But yeah, DI and DIP are just a liiiiiittle too close in acronym-land.

  • Rock Ge

    Great write up! Points were clear and codes are simple!

    I can see NerdDinner becoming the next northwind. :)

  • http://thomaseyde.blogspot.com Thomas Eyde

    @Mike

    I will look into the Suteki code when I find the time. Sounds interesting.

    And I was thinking that this is something we are forced to do when we decide to expose the DataContext. Which means our repository code now got more complex. In Suteki Shop, that’s propably what you need. But for a demonstration like Nerd Dinner, that’s overkill.

    There is a finer point here, which I continously fail to articulate: Which types of dependencies should we expose, and which ones are ok to keep hidden? As we have seen here, some exposures come with a significant cost.

    I am still of the opinion that a repository implementation is infrastructure code, while the repository contract is domain code. I don’t think infrastructure implementation should leak into the domain.

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Thomas

    It is a fine line to be sure – not every private field should be exposed. I have places where I keep a local cache in a dictionary – that shouldn’t be exposed. For me personally, my line is “does component X require component/service Y to fulfill its responsibilities”. I think the component/service distinction is important, too.

  • http://devlicio.us/blogs/anne_epstein/ Anne Epstein

    Brendan addressed this, but I really think it’s worth emphasizing that though it would certainly be an antipattern to write *new* code like this, “poor man’s DI” can be really useful in situations with a significant established codebase that was written using parameterless constructors.. In those codebases, it’s the no-arg constructor that’s the original, primary constructor. Writing the add-on DI-friendly constructor in addition to the existing no-args constructor is a way work in testability without having to touch the rest of the codebase, allows new code to use the DI-friendly constructor, and provides a target for future refactoring of the old code to use the new constructor when possible. (hopefully very soon!)

  • http://rob@wekeroad.com Rob Conery

    Hi guys – I’m the one who wrote this so I’ll speak to why it was done this way. First I’m going to defend what we did: it’s not wrong if it works :) and it works.

    I did raise the point to “the Scotts” about the coupling issue – that DI will allow you know only the interface and not the implementation but the general feeling was that we’re not building the Stock Exchange and to try to introduce the concept of IoC while at the same time talking about DI seemed just a tad silly.

    Sure using StructureMap is “better” – you know I agree with that. But the thing you’re missing here is understanding the pattern before improving it and if you were to read the tutorial I wrote exactly that: “we have an issue with this constructor …” but my guess is you didn’t. That’s OK, I’m used to it :)

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Rob

    Don’t worry – I was told about 80 times after I posted this that you guys explained this in the material.

    This _is_ a valid pattern, as are all, and I think I need to explain where each is valid.

  • http://www.patmaddox.com Pat Maddox

    I think it’s funny how you managed to up the code size by 10x without making it better, and still call it better. The code is tightly coupled? Uh, no. Just pass something in via the 1-arg constructor

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Pat

    Yes, my IDE nearly exploded with the weight of new code added to employ DI. I’m not sure how my machine was able to handle the extra order of magnitude of code created to support dependency injection.

    Poor man’s DI requires _more_ coding, not less, as the example above showed. Poor man’s DI is by definition more coupled, as I am coupled to specific implementations, as opposed to abstract interfaces. I highly recommend you check out Uncle Bob’s Agile Software Development book, he explains far better than I how following the SOLID design principles improves your code.

  • Pingback: IoC vs ServiceLocator | Contexto

  • Pingback: Inyectando dependencias en Windows 8 Metro | Kinetica Solutions Blogs