Ayende @ Rahien

It's a girl

Review: Microsoft N Layer App Sample, Part IV-IoC FTW

Continuing my review of http://microsoftnlayerapp.codeplex.com/, an Official Guidance (shows up at: http://msdn.microsoft.com/es-es/architecture/en) which people are expected to read and follow.

While investigating how they are implementing IoC, I found:

/// <summary>
/// Configure root container.Register types and life time managers for unity builder process
/// </summary>
/// <param name="container">Container to configure</param>
void ConfigureRootContainer(IUnityContainer container)
{
    // Take into account that Types and Mappings registration could be also done using the UNITY XML configuration
    //But we prefer doing it here (C# code) because we'll catch errors at compiling time instead execution time, 
    //if any type has been written wrong.

    //Register Repositories mappings
    container.RegisterType<IProductRepository, ProductRepository>(new TransientLifetimeManager());
    container.RegisterType<IOrderRepository, OrderRepository>(new TransientLifetimeManager());
    container.RegisterType<IBankAccountRepository, BankAccountRepository>(new TransientLifetimeManager());
    container.RegisterType<ICustomerRepository, CustomerRepository>(new TransientLifetimeManager());
    container.RegisterType<ICountryRepository, CountryRepository>(new TransientLifetimeManager());

    //Register application services mappings

    container.RegisterType<ISalesManagementService, SalesManagementService>(new TransientLifetimeManager());
    container.RegisterType<ICustomerManagementService, CustomerManagementService>(new TransientLifetimeManager());
    container.RegisterType<IBankingManagementService, BankingManagementService>(new TransientLifetimeManager());
    
    //Register domain services mappings
    container.RegisterType<IBankTransferDomainService, BankTransferDomainService>(new TransientLifetimeManager());
    

    //Register crosscuting mappings
    container.RegisterType<ITraceManager, TraceManager>(new TransientLifetimeManager());

}

Just to figure out how ridiculous this is, let me show you the entire infrastructure required to support IoC in this application:

image

Yes, they abstracted the Inversion of Control Container. I think that if you need to do that, it is pretty clear that you don’t really get what IoC is all about.

Maybe, if you are a framework, you need to abstract the container, but you don't need to do that if you are an application (which this is supposed to be) and you certainly don't write your own, that is why CommonServiceLocator is here.

Then again, the code is absolutely littered with things like:

image

So I guess that it is not really a surprise.

What is a surprise is that they are catching NullReferenceException. You are not supposed to catch this exception. This is an indication that you have some problem with your code, not that you have some invalid argument issue.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Torkel
07/04/2011 09:15 AM by
Torkel

Wow, that is the ugliest error handling I have seen in a long time, can't believe that it is included in an official sample app. And to catch NullReferenceException? Insane.

Scooletz
07/04/2011 09:20 AM by
Scooletz

Having plenty of Resolve calls eventually makes all your methods static, even if they're not marked as ones. It's a procedural programming.

Scooletz
07/04/2011 09:21 AM by
Scooletz

The second thought: why on earth Unity.Interception is not used to handle it?

Lee
07/04/2011 09:27 AM by
Lee

why isn't ITraceManager a ctor parameter!?

dario-g
07/04/2011 09:29 AM by
dario-g

Why Microsoft allowed that officially?

PS Tags for this note is not in place :)

Dave
07/04/2011 09:48 AM by
Dave

That's what's worries you? There is no check if pagedCriteria is not null (code contracts?). Then they started to pass invidual parameters instead of just passing the pagedCriteria instance. To me that shows they don't even understand OOP.

Because they don't check for a nulled pagedCriteria, they don't know if they're catching customerService null exception or a nulled pagedCriteria exception. Looking at the catch code, they assume there is always a customerService returned by the IoC container.

Maybe it's time Microsoft also starts using GIT so the community can actively patch the official microsoft code with proper coding so people looking for guidance can actually learn something from it.

Tudor
07/04/2011 09:56 AM by
Tudor

It's somehow expected that the code is littered with calls to the service locator, since most examples and documentation about Unity from Microsoft always shows calls to Resolve<...>: http://msdn.microsoft.com/en-us/library/cc816062.aspx#MainExample or Get<...> : http://www.pnpguidance.net/post/UnityDependencyInjectionContainerEnterpriseLibrary4.aspx so most people don't even know about constructor injection...

Marta
07/04/2011 09:56 AM by
Marta

Their have a great idea! You're all good to criticize! Why do not you raealizzate it? Why not help them to improve the sample application and the core framework instead jabberting? I hate the criticism ends in themselves! Regards. M.

Bogdan Marian
07/04/2011 10:05 AM by
Bogdan Marian

@Tudor Since I have only seen Container.Get<...> or Container.Resolve<...>, what would be the correct usage for Unity? I know what ctor injection is, just do not know how to apply it inside Unity ...

NC
07/04/2011 10:27 AM by
NC

@Marta - Ayende has already given the project owners early access to all his posts and they are already making changes to rectify the issues he has raised.

While I think it's great for Ayende to highlight these sort of issues, I still wish he would provide an alternative way to help educate people.

Marta
07/04/2011 10:52 AM by
Marta

@NC - Sorry, but the tone used in all posts and in all comments is not from a person who wants to help!

David Turner
07/04/2011 11:55 AM by
David Turner

Official guidance from Microsoft (especially the Enterprise Library crowd and their brethren) often reminds me of that song from The Mikado:

"On this subject we pray you be dumb (dumb dumb) Your words, though they're many, are not worth a penny..."

Pablo Alarcón
07/04/2011 12:07 PM by
Pablo Alarcón

I think most people is not realizing that the sample code shown is a WCF Service, so there is a lot of additional complexity involved in using Dependency Injection: WCF Behaviours, IInstanceProviders and WCF ServiceHosts are involved if I'm not wrong.

If you have a look in "any other" layer in the app Constructor Injection is used, probably Property Injection too.

In fact, a WCF Service could be seen as an "application entry point", the same way a Controller is in MVC. The key point is that those Resolve() should be encapsulated in the WCF Infraestructure, the same way as it is Resolved in MVC's DependencyResolver or ControllerFactory..... but it is much harder in WCF.

PS: Jimmy Bogard has this post about Resolving WCF Services with a IoC Container(StructureMap): http://lostechies.com/jimmybogard/2008/07/30/integrating-structuremap-with-wcf/

Tudor
07/04/2011 12:15 PM by
Tudor

@Bogdan: In Unity you use constructor injection like in any other DI framework, ex.: http://softarchitect.wordpress.com/2011/01/16/software-architecture-dependency-injection-with-unity-for-constructor-injection/ Of course, the "root" instance (ex.: a 'bootstraper') will have to be created using a service locator, but that's it..

Justin A
07/04/2011 12:34 PM by
Justin A

Word of the day: jabberting (cheers @Marta)

Steve
07/04/2011 12:39 PM by
Steve

Dude, Your rant makes no sense! CommonServiceLocator does exactly what you are saying you should not do. There is nothing wrong with abstracting the IOC implementation ! In fact it's good practice ! Resolve is sometimes in lined deliberately for a good reason despite knowing about constructor based injection! In those cases abstraction is necessary for testability! Perhaps one should remove ones head from ones bottom !

Marta
07/04/2011 12:39 PM by
Marta

@ Justin A - it is my slang :-P ...I mean "to jabber" ...sorry for my english :-)

Kash
07/04/2011 12:40 PM by
Kash

@Pablo Alarcón

This is correct. You can find examples of regular services with constructor injection in the codebase here http://microsoftnlayerapp.codeplex.com/SourceControl/changeset/view/54267#943263 and here http://microsoftnlayerapp.codeplex.com/SourceControl/changeset/view/54267#943255 and in more places, then the sentence "Then again, the code is absolutely littered with things like:" is false, except for WCF service methods.

The method GetPagedCustomer showed in the post is a WCF service method, and I think that (IMHO) for simplifying, authors haven't built an IoC infrastructure compatible with WCF.

Ayende Rahien
07/04/2011 12:40 PM by
Ayende Rahien

Steve, Did you miss: Maybe, if you are a framework, you need to abstract the container, but you don't need to do that if you are an application

Steve
07/04/2011 12:47 PM by
Steve

I think that the same principle applies for both application and framework code: For testability abstraction is necessary.

Tudor
07/04/2011 12:57 PM by
Tudor

@Steve - that's the idea - in a custom application you don't usually swap the container or service locator with another one, and you don't need to create mocks for it (only for the classes that are resolved using the container)..

Also, in general using a service locator in too many places in an application is seen as an anti-pattern (because the dependencies are not explicit)

Kash
07/04/2011 01:02 PM by
Kash

I think the same as Steve, independent of the code purpose, IOC container abstraction is a very good practice, not only for testing, but for allow future changes for the IoC technology too.

Guilherme
07/04/2011 01:03 PM by
Guilherme

Is there anything good in this sample app?

Xexu
07/04/2011 01:38 PM by
Xexu

@Kash No it's not. Actually is the first time I see an IoC abstraction. I'd rather to see a IoC Controller factory automatically resolving dependencies like n MVC framework. You don't usually change your IoC that much. Although in this case they are using this IoC as a single factory, there isn't automation. Finally handling a null-reference exception it's definitively a very bad idea. You'll end up swallowing infrastructure errors.

Rob Ashton
07/04/2011 03:03 PM by
Rob Ashton

Guys - you shouldn't need to abstract the IOC container because you shouldn't be calling into it from anywhere but the bootstrapper of the application - and if you want to test that aspect of it it's an integration test.

Really you don't even need an IOC container for this, you should (up until the point you have any dynamic behavior) be able to simply construct your object graph up front manually if you so wish (gasp, I know).

If you're wanting to mock your container all over the place then you're most likely doing wrong. If you're wanting to mock any interface other than one you own, you're most likely doing it wrong. If you're creating one-to-one interface wrappers around somebody else's code so you don't break the previous rule, you're most likely doing it wrong.

Abstracting something like this doesn't automatically mean you've reduced the coupling between anything, abstracting something like this doesn't automatically mean you even have testable code, abstracting just means you have code where you previously had none.

Abed
07/04/2011 03:45 PM by
Abed

The only valid reason for me to abstract IoC container is the flexibility which allows you to switch IoC framework, maybe you find a better framework in terms of performance or whatever, instead of breaking code you can switch ur IoC framework using this level of abstraction.

BTW a similary way is used in WPF Composite Application (Prism). and it is giving you the flexibility of plugging whatever IoC framework you need.

about calling it as service locator, yes it is a bad idea but given that it is only used in wcf as bootstrapping instead of WCF Behaviours, IInstanceProviders and WCF ServiceHosts. Also it could be used in unit testing when you need to resolve some type explicitly.

No doubt the null reference exception is the wrongest implementation.

Steve
07/04/2011 03:48 PM by
Steve

Whilst i agree with everything Rob and co is saying, this is very much a utopian and academic argument. In practice IOC implementations are occasionally swapped out and sometimes you just don't want to resolve a dependency until the very last minute. eg command execution and sometimes using resolve inline is simply unavoidable.

I am coming from a very pragmatic point of view: My point is simple: IF you are resolving dependencies inline then you MUST either abstract your IOC implementation or push back to the preferred constructor based injection. Having tech specific code all over the place is simply unacceptable.

Steve
07/04/2011 03:56 PM by
Steve

Yeah just for the record, in fact it goes without saying that the exception catching in this sample is horrific !

Alex Simkin
07/04/2011 04:16 PM by
Alex Simkin

OK - they just did somethinf to fix IoC. I have a question - how something with so many commits per day and constant major refactorings could be called "a gidance". Do you want tofollow a guide who runs around a town like crazy?

KallDrexx
07/04/2011 04:31 PM by
KallDrexx

Maybe I am missing something, but what is wrong with the IoC resolve calls?

In my application I created a class calls my IoC container and registers types on demand like this (I guess this could technically be considered abstracting out the IoC out). The reason I did this was because I use the command pattern in my application (each query or action is a separate class), and thus my MVC constructors were becoming huge even when a specific action may only need one command. This was inefficient, made unit testing annoying when I had to add more commands to a MVC controller. Also, it means that the IoC has to instantiate a lot of classes that are never going to be used.

Exactly what I implemented can be seen at http://scatteredcode.wordpress.com/2011/05/29/keeping-asp-net-mvc-controller-constructors-clean-in-a-dependency-injection-world/ and if there are any criticisms on why this is bad I would definitely like to know.

Ayende Rahien
07/04/2011 04:51 PM by
Ayende Rahien

Alex, The had a released version, which is what I review. I am assuming that they are also following up on the posts and responding to them.

Ayende Rahien
07/04/2011 04:53 PM by
Ayende Rahien

Kall, You now lost the ability to have the container tell you about dependencies. You are much better off with using properties injection instead.

Kash
07/04/2011 04:59 PM by
Kash

@Alex Simkin

Just now, V2.0 is being developed, which is in alpha state, and v1.0 have very few changes since its launch in November 2010. The changes your are seeing belong to version 2.0

SteveR
07/04/2011 05:04 PM by
SteveR

@KallDrexx Rob Ashton, who commented above, has a post which touches on controller bloat( http://codebetter.com/robashton/2011/06/13/finding-a-balance-with-asp-net-mv/ ). Custom model binding would be resolving the right command/query model and pushing it into your action methods rather than you pulling them out of the IoC container in your action methods or having them injected into the controller's constructor. There's some posts elsewhere about using model binders and IoC which I'm sure you can find. The outcome would be that your IoC usage would be in infrastructure code where it belongs.

Alex Simkin
07/04/2011 05:23 PM by
Alex Simkin

@Kash

If you call something a guidance, it means that you know they way and you guide others to help them to avoid landmines that you have discovered already. It doesn't mean that you have no clue and do a BDD (blog driven development) in front of the others.

KallDrexx
07/04/2011 05:29 PM by
KallDrexx

@SteveR: I like the idea from that article of separating out 1 controller per action, that seems like an interesting idea (although that will require a lot of routing code to make sure I don't break existing links in my application). However, I don't like the way he uses his models to perform the business logic as it seems to require the controllers themselves to perform the data access. Instead I prefer my service classes to perform all the calls to the data layer, as that also makes it easier to contain all my business and data logic outside of the MVC architecture. It also means I don't have to rewrite my data access calls if I extend my application out to a WCF or Silverlight basis. Unless I am misreading that post at least.

KallDrexx
07/04/2011 05:38 PM by
KallDrexx

@Ayende, I just figured out what you meant by property injection. I honestly didn't know that was possible. That makes life so much easier, and is a much better solution.

I just need to find a way to mark the properties so that Windsor will error if it can't resolve them.

SteveR
07/04/2011 05:39 PM by
SteveR

@KallDrexx: I'll reply on your blog rather than leech Ayende's bandwidth. ;)

Ayende Rahien
07/04/2011 05:41 PM by
Ayende Rahien

Kall, I can't recall of it off hand, but I think you can make it do so by playing with: PropertiesDependenciesModelInspector

tobi
07/04/2011 10:06 PM by
tobi

Omg what an abomination! Look at the spelling the the comments of the NullRef handler. This code was written in India.

I just noticed that, in addition to being in the wrong place, the code in the two catch clauses is the same. I cannot believe this.

What a disgrace!

David
07/05/2011 12:14 AM by
David

Microsoft + Guidance = Oxymoron

Johan
07/05/2011 02:31 AM by
Johan

Rob Ashton has it exactly right.

When I see .Resolve() calls, I immediately know I am dealing with someone who just found out about IoC, and hasn't yet discovered how much simpler constructor or property injection makes your code.

How is passing the container around to every method and calling Resolve() any different to singleton GetInstance() sprinkled through the application.

If you use a non-crap IoC container like StructureMap or Autofac, constructor & property injection Just Works.

Then, you have a "hard dependency" on your container only in the bootstrap code which is usually one method, and not in 50 million places in your code base.

Then, you don't need to abstract the f***ing thing.

And your tests are simple because you're only mocking the ACTUAL dependencies not the whole f****ing container.

:)

Johan
07/05/2011 02:33 AM by
Johan

Lest anyone thing I'm being a bit harsh...

I made exactly these same mistakes the first time I used IoC, and over time I learned from these mistakes :)

VladB
07/05/2011 04:32 AM by
VladB

@Steve: When you have to resolve a dependency in the very last minute you should use factory pattern, not IoC-container itself.

IoC-container is intended to be used in only one place to build object graph and resolve dependecies. You are about to get rid of IoC-container after that.

steve
07/05/2011 06:43 AM by
steve

VladB: By using an IoC container you are using the factory pattern ! That's the whole point ! I am not going to create yet another factory !

Ayende: Property injection breaks encapsulation. Interesting that you would sacrifice good OO code for some idealism of not calling Resolve on your container directly.

Guys I agree with the resolve point in principle, but like i keep saying; calling resolve is a last resort and for me property injection is the very last thing i would do !

Ayende Rahien
07/05/2011 08:22 AM by
Ayende Rahien

Steve, Property Injection means that you still have the container manage the dependencies. It actually uses the container. Using Resolve all over the place means that you have broken that, making a lot of the features of the container useless.

Tudor
07/05/2011 09:40 AM by
Tudor

Another reason why many people use a service locator and access the container directly to resolve dependencies is that they create very coupled classes with many dependencies - so instead of having one constructor with 40 arguments some think it's more 'elegant' to spread the Resolve<> class in various methods.. :)

steve
07/05/2011 09:58 AM by
steve

Ayende, I think we will have to agree to disagree on that. Using resolve still uses the container I don't see the difference. Property injection breaks encapsulation, is harder to understand and results in spaghetti code. I would rather have a property who's Get method calls resolve (on an abstraction) for readability than have public properties all over the place. But let me re iterate; Using Resolve should be avoided for all the reasons people have mentioned but when in the situation where you can't avoid it then I don't think property injection is an acceptable answer.

steve
07/05/2011 10:08 AM by
steve

Tudor, too many dependencies is not a reason to use resolve rather than constructor based injection. That is also unacceptable, the class should instead be refactored to smaller classes!

Guys my decisions with regard to things like this are driven by S.O.L.I.D and Clean principles. I think we sometimes forget that we are part of a team and as a result do not consider fellow developers. Often as a consultant coming into a project, it's too easy to just do whatever to get the job done (property injection) because you know you won't be around to debug it when it goes wrong ! I think part of being a good developer is less about writing complex code but about writing code that reads like a book to other developers, code that is maintanable and easy to change and code that is 'elegant'.

steve
07/05/2011 10:13 AM by
steve

Ayende, Just for the record I still think your awesome ! But I do enjoy a technical debate !

Adrian
07/05/2011 09:00 PM by
Adrian

"Guys my decisions with regard to things like this are driven by S.O.L.I.D and Clean principles. I think we sometimes forget that we are part of a team and as a result do not consider fellow developers"

Oh really? And have you ever asked your fellow developers what they think when one of your Resolve calls blows up in production because...oh...you didn't know about it and didn't register the implementation? Solid!

Jay
07/05/2011 10:01 PM by
Jay

I've noticed with several of Ayende's articles, that he seems to be writing to an audience that already understands what he is saying and agrees with him. This is not useful to the novice that may want to learn from him and avoid the mistakes he seems to be pointing out. Comments such as " it is pretty clear that you don’t really get what IoC is all about" should be backed up with some sort of justification or at least a link to an article that explains what the problem is. Same thing with why it is a bad idea to litter your code with Factory calls. Why is this bad? What is better?

Chuck
07/06/2011 12:55 AM by
Chuck

These post by Ayende often frustrate me because they can challenge my status quo! I've abstracted my container just to demonstrate that I can change it, but, for a day to day application, I change containers about as often as I change my database - it just doesn't happen as much as I want to think it will.

Ayende Rahien
07/06/2011 05:30 AM by
Ayende Rahien

Jay, You mean, like the thousands of posts on this blog? My IoC category alone contains dozens of them.

Tudor
07/06/2011 07:13 AM by
Tudor

@steve

"Tudor, too many dependencies is not a reason to use resolve rather than constructor based injection. That is also unacceptable, the class should instead be refactored to smaller classes!"

I agree with you - I just described a reality, did not say that it's good to do that way.. The path to SOLID principles, 'clean code' and refactoring is a long and difficult one for many developers, so in a less than ideal world many compromises should be done, unfortunately (f course, this is not an excuse for a guidance such as this)..

I've worked this year with architects from Microsoft, in a project, and some of them still don't make the difference between proper unit testing and integration tests, think that the proper way to implement a logger class is with static classes and don't even consider using DI or IoC in a project..

steve
07/06/2011 09:57 AM by
steve

Adrian: "Oh really? And have you ever asked your fellow developers what they think when one of your Resolve calls blows up in production because...oh...you didn't know about it and didn't register the implementation? Solid! "

LOL ! Dude thats my point ! Imagine how much more difficult that would be to debug if it was being set up in application configuration using property based injecttion. Yeah real solid ! Besides that doesnt happen becuase you test your application before you deploy. You do test your applicaiton before you deploy dont you? oops !.

Using complicated Ioc techniques negates the whole point of Ioc in the first place: To simplify the development process !

Hendry Luk
07/07/2011 03:19 AM by
Hendry Luk

@Kash. You don't abstract IoC container. You might want to abstract service-locator, but no you don't abstract IoC container. To swap the container with other technology, you can just do that right away without the need for any abstraction, due to the very nature of IoC pattern.

Hendry Luk
07/07/2011 03:40 AM by
Hendry Luk

I'm completely with you Ayende. It's rather worrying when you see code like this end up in an official design guidline. It's extremely hard to imagine this is an official guideline produced by the same organisation who wrote asp.net MVC, MEF, Prism Rx, etc. It just shows an apparent fracture in the believe-systems among different teams at Microsoft.

Per Erik Stendahl
07/07/2011 10:32 AM by
Per Erik Stendahl

Wow, 12 lines of error handling infrastructure for 2 lines of application logic. Very nice discussion though.

Ayende, here's a suggestion for you: make a wiki for these posts so people can post their own code suggestions and discuss it!

Ayende Rahien
07/07/2011 10:33 AM by
Ayende Rahien

Per, I like that it is in the blog

Abed
07/07/2011 10:38 AM by
Abed

Note that this application (specially V2) is still in draft phase, and if you see the frequency of refactoring and changes you will notice that it is still far from being complete.

Even the point we are discussing about the IoC used as service locator from WCF remote facade, they changed it already a couple of days ago, added IInstanceProvider in WCF for injecting dependencies and removed IoC project.

So I guess we can delay full judgement till the release. Still it is very beneficial to discuss the aspect while it is still in development phase but without much expectation of finished work.

Ayende Rahien
07/07/2011 10:40 AM by
Ayende Rahien

Abed, I am not posting about the pre-release version, I am posting about the actual release, Official Guidance version, which contains all of those problems.

Abed
07/07/2011 11:16 AM by
Abed

Then you got a point, i thought you are posting about the new version.

keep up the good job of reviewing this guidance ... anyway it is a good step already that microsoft officially trys to support DDD style application instead of their traditional data oriented anemic MS-TLSA style applications.

flukus
07/08/2011 09:36 AM by
flukus

I'd find this funny if I didn't have to spend 6 months of my life working on a code base much like it.

I don't follow the law of demeter very closely but all those resolve calls seem to be a pretty clear violation.

I also don't understand abstracting the container. One of the things about IOC that always amazed me was how such a fundamental part of an application can be completely replaced in a matter of hours.

Paul Hiles
07/16/2011 06:29 AM by
Paul Hiles

For anyone still struggling to see why (1) it is bad practice to call Resolve from anywhere but the root of your application and (2) abstracting your IoC container is generally unnecessary, this article (series) might help:

http://www.devtrends.co.uk/blog/how-not-to-do-dependency-injection-the-static-or-singleton-container

Comments have been closed on this topic.