+972 52-548-6969
ReviewMicrosoft 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:
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:
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.
Comments
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.
Having plenty of Resolve calls eventually makes all your methods static, even if they're not marked as ones. It's a procedural programming.
The second thought: why on earth Unity.Interception is not used to handle it?
why isn't ITraceManager a ctor parameter!?
Why Microsoft allowed that officially?
PS Tags for this note is not in place :)
Dario, Opps! Fixed!
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.
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...
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.
@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 ...
@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.
@NC - Sorry, but the tone used in all posts and in all comments is not from a person who wants to help!
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..."
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/
@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..
Word of the day: jabberting (cheers @Marta)
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 !
@ Justin A - it is my slang :-P ...I mean "to jabber" ...sorry for my english :-)
@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.
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
I think that the same principle applies for both application and framework code: For testability abstraction is necessary.
@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)
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.
Is there anything good in this sample app?
@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.
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.
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.
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.
Yeah just for the record, in fact it goes without saying that the exception catching in this sample is horrific !
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?
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.
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.
Kall, You now lost the ability to have the container tell you about dependencies. You are much better off with using properties injection instead.
@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
@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.
@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.
@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.
@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.
@KallDrexx: I'll reply on your blog rather than leech Ayende's bandwidth. ;)
Kall, I can't recall of it off hand, but I think you can make it do so by playing with: PropertiesDependenciesModelInspector
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!
Microsoft + Guidance = Oxymoron
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.
:)
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 :)
@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.
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 !
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.
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.. :)
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.
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'.
Ayende, Just for the record I still think your awesome ! But I do enjoy a technical debate !
"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!
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?
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.
Jay, You mean, like the thousands of posts on this blog? My IoC category alone contains dozens of them.
@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..
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 !
@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.
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.
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!
Per, I like that it is in the blog
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.
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.
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.
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.
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
Comment preview