http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660@EricP I also did not get everything what Udi was talking about, his world is quite hermetic but at least I understood some basic concepts. His idea to deal with too many inter-service dependencies was to let each service take care of everything, from the database to GUI. Moving this analogy to ASP.Net MVC world, instead of trying to integrate all services inside one huge controller one should implement a GUI 'pagelet' for each service and then create a mashup of all these portlets on the web page. So you would create a web page that would contain several embedded 'pagelets' like the basket view, discounts, product list, recommendations and so on. Yes, I know this is not a ready solution, rather replacing old problems with new ones.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment50http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment50Thu, 20 Oct 2011 12:06:07 GMTHey Frank, I think know what you mean and I've seen many code samples doing it the way you describe. The problem I have then is that it encapsulates my dependencies so I can't see which controller uses what commands. So when another developer uses my controller, he has no direct view on the dependencies that the controller has. In other words you need to have a birds eye view of the dependency tree in order to avoid making mistakes.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment49http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment49Thu, 20 Oct 2011 10:26:26 GMT@Rafal I took a look at the article. Actually skimmed through it. Not sure how "Command Query Responsibility Segregation" applies to building UI in MVC environment, without seeing any actual code samples. I am not too fund of articles written for developers that have not code examples. No offence to Udi.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment48http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment48Thu, 20 Oct 2011 10:00:34 GMT@EricP Udi Dahan made a post about CQRS recently and he was discussing the pattern you described (see http://www.udidahan.com/2011/10/02/why-you-should-be-using-cqrs-almost-everywhere%E2%80%A6/). It was an interesting read and, although I don't agree with everything he said, some of his ideas could be useful in your case: instead of tying all these services together in the controller you could make them provide relevant GUI fragments and integrate everything at the browser. Rhttp://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment47http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment47Thu, 20 Oct 2011 09:26:33 GMTAren't you overestimating the importance of code structure vs all other qualities of the software? I know, this is about a code review, but almost all people here, including the Author, jump to strong conclusions after just a glimpse at some fragments of the code (or what's worse, just at the constructor in discussion). 'Pain', 'Hard to maintain', 'WTF', 'complex', 'complicated' - wow people this makes me think you haven't really seen complex and hard to maintain code. Im not saying this code is beautifu bcause I haven't seen it, but if the application is a success who cares about the code? http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment46http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment46Thu, 20 Oct 2011 09:20:18 GMTThe following blog post from Mark Seemann describes how to handle such situations: http://blog.ploeh.dk/2010/02/02/RefactoringToAggregateServices.aspxhttp://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment45http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment45Thu, 20 Oct 2011 09:18:27 GMTI used Service injection in a project recently. Some of the pages were pretty complex and required displaying information from various parts of the system. So many controllers ended up looking similar to above. The example above includes many infrustructure level services which shouldn't be on controller (like caching, email, etc)... In my project to Create Order and Place Order I would need: - Display list of Products person can add to order - ProductService - Display list of available Discounts/Coupons - DiscountService - Be able to retrieve existing Customer, display Customer Info - CustomerService - Be able to place Order - OrderService So 4 services just of the top of my head. I am sure I can come up with a bunch more. So how would you deal with this without injection all these services into controller? http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment44http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment44Thu, 20 Oct 2011 09:11:53 GMTLuc, can you not have essentially an IPublisher interface and publish your command, which allows you to get to the right command handler in your infrastructure instead of forcing your code to choose the right one?http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment43http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment43Thu, 20 Oct 2011 09:10:11 GMTDefinitely smells of violating SRP; does a single class need all those dependencies ? Some look like candidates for implementation as aspects using (say) PostSharp as well. Looking forward to seeing what turns up next..http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment42http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment42Thu, 20 Oct 2011 08:26:02 GMTI don't understand why it's so hard to produce a good example of how to handle this. Order processing is a very common use case-- could one of you please bless us with your fancy schmancy DDD design? Instead everyone is just pooh poohing it--- it's not helpful to just spew "Fat Controller", "Low Cohesion", "Anemic Domain Model", etc. Ayende, what ever happened to Macto? I was looking forward to it. http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment41http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment41Thu, 20 Oct 2011 05:55:47 GMTHey Ayende, In a project I'm doing right now we use the CQS pattern. The problem is that we have allot of command/handler objects that need to be injected into the constructor of our controllers. A small sample would be like: public SomeController(CreateCommandHandler createCommandHandler, UpdateAddressHandler updateAddressHandler, UpdateFunctionHandler updateFunctionHandler, ... I've had controllers that took more than 12 of these handlers in the constructor because there is a commandhandler for each operation. Do you think it's bad practice injecting all these handlers? A friend of mine suggested that I should use a factory object for this so that I don't have inject all the handlers into my constructor, but this way of working seems allot like the dependency resolver anti pattern. Thx http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment40http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment40Thu, 20 Oct 2011 04:58:38 GMT(two cents) i would almost bet that who ever wrote this did so with a "framework" in mind thinking that all of these services can be simply swapped out and changed easily either that or they just got done reading the "loosely coupled" chapter ahhh... maybe all of these are actually interfaces to web services (thus implying they just got done reading the "SOA" or "scalability" chapter) @bugeo - the problem isn't with the constructor but with the number of services required for a single controller. simply wrapping the services within a "service" wrapper only adds to the smell @Travis - you might very well see a lot of those types of comments without any "do this instead" comments. i think this has more to do with the fact that this isn't our bloghttp://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment39http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment39Thu, 20 Oct 2011 04:21:53 GMTJustin - I think that technique is picked up by DBA's, the only time I've ever seen that is when people write SQL like that. http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment38http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment38Thu, 20 Oct 2011 00:19:24 GMTI'm shocked and surprised no one has picked up on this sooner! It's because each constructor argument -line- starts with a comma then space. like WTF?! There's a great disturbance in the Force, as if millions of voices suddenly cried out in terror, and were suddenly silenced. Thank you, commas-in-the-wrong-spot. :~(http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment37http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment37Wed, 19 Oct 2011 23:23:40 GMTTobi, MVC is a UI pattern. Controllers should not be responsible (directly) for sending email. That should be done in the infrastructure layer.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment36http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment36Wed, 19 Oct 2011 22:09:54 GMTtotally agree with Bartek, they need to build better abstractions, put some of those services behind other services, not perform the logic in the controller, that's what that looks like, logic in controller, firing actions in the "services", when a better approach would be to have fewer services that handle the logic and perform actions based on that (email, cripto, cache, don't seem to make much sense in the context of a controller)http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment35http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment35Wed, 19 Oct 2011 19:43:12 GMTI must say that the comments in this post are an amalgamation of different opinions sold as being always true. It is really not possible to have a best practice that is always true.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment34http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment34Wed, 19 Oct 2011 19:04:21 GMTNaming a class "XxxService" is bad practice, its like all those manager classes.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment33http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment33Wed, 19 Oct 2011 18:29:18 GMTI don't think business logic in a controller is a bad thing by itself. It is a convenient place to put it, iff it causes no architectural harm (for example if it is trivial like sending a mail on save).http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment32http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment32Wed, 19 Oct 2011 17:54:51 GMTAs for the "instead, do it this way", I would suggest that a presentation layer class with that many injected services probably ought to be split into multiple, more focused presentation layer classes. Of course it's impossible to say that definitively without looking at the implementation and requirements, but that's a lot of services for a single class to need in order to have one clear responsibility.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment31http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment31Wed, 19 Oct 2011 16:19:45 GMT@Stacey, I'm curious about this part: "nested classes are strongly discouraged by .NET 'best practices'". Let me ask one thing for you: Think that your model demands an object composition (I'm not talking about aggregation here, it's simple OO composition). How do you represent this on your code?http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment30http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment30Wed, 19 Oct 2011 15:42:31 GMTBy the way, judging by the ctor signature, the controller should be decomposed. However, the URL conventions of ASP.NET MVC might require that this particular controller class still exist, but that doesn't mean decomposition can't take place. One could refactor the logic into "sub-controllers" that this controller will depend on. Interestingly, the result appears similar to the result of composing more coarsely-grained services from the existing services. However the process differs and that is significant. Decomposing a class leads to dependencies that are probably relevant only to that class. Composing coarsely-grained services from the existing services should be driven by the broader business context, and should result in services that have broader application.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment29http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment29Wed, 19 Oct 2011 15:38:34 GMTThe problem with Best Practices is that they discourage programmers from understanding the scenario they are prescribed for. "If it's the *best* practice, I don't even need to think about how I design my code." Of course the truth is that it's only "best" for specific scenarios. Programmers either A) fail to learn this, B) are victims of incomplete or poor documentation by the practice's advocates, or C) are simply enamored by the term "best practice". Over time, Best Practices are blindly followed, producing the best practice cargo cult that is endemic in many "enterprise" software houses today. Not to troll, but as alluded to in recent discussions here, the programmers that are most susceptible to this are the ones that aren't passionate enough about their craft to scratch beneath the surface of best practices and learn them deeply. This population correlates highly with those programmers that don't program/study in their personal time. Unfortunately, this is validated by capitalism because most software patrons do not know high quality software from low quality, and if they do, many do not care to pay extra for it. And this problem is much the same as the "how do I trust my car mechanic?" problem. Until we solve that problem, we are stuck in a world of cargo cult programmers.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment28http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment28Wed, 19 Oct 2011 15:28:45 GMTAt first glance, it is like the code using dependency injection. the framework may force programmer to inject those dependency service/persistent objects into controller. When controller need more services, it becomes a god controller. I agree with bugeo to centralize all services into a factory object. To this kind case, are we still need IoC container for this object, since we may end up with the IoC only contains this single object.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment27http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment27Wed, 19 Oct 2011 15:22:57 GMTTravis, the reason you won't see any "instead they should do it this way" is because without a proper domain analysis, it's not practical to exactly how it should be done. (Or it's simply a standard online storefront, in which case "they should do it this way" is practically useless, since the design patterns for that are nearly ubiquitous.) I fear that StoreController interface, though. That's either a useless abstraction (since it looks like OrderController is going to do everything under the sun anyway), or - far more frightening - there's other "God" classes inheriting StoreController. http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment26http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment26Wed, 19 Oct 2011 15:12:23 GMT@Ayende, I am looking forward to your explanation. To be honest i was running into same problems, with such over-grown controllers as well. I was trying to be able to write unit tests for my controllers without a need of IoC or DI libraries. Anyway - looking forward to you ideas. http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment25http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment25Wed, 19 Oct 2011 15:01:36 GMTI foresee gallons of "this sucks, they are doing it wrong" and absolutely NO "instead they should do it this way" in this series.http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment24http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment24Wed, 19 Oct 2011 14:20:26 GMT@dabuddhaman whether you do property injection (adjustment) or ctor injection (dependency) is beyond the issue - the issue is a high amount of coupling...how come one class needs to know about so many things to operate? Does the controller really need so many collaborators? I assume many of these interfaces are passed to other "services" in order for them to operate. In that case, the receiving thingy could have been created and passed in without the controller passing that dependency. Partial application of delegates or using a decorator pattern can help achieve this. http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment23http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment23Wed, 19 Oct 2011 13:51:50 GMT@dabuddhaman, That doesn't actually solve the problem, it just moves and hides it. The issue isn't the number of objects taken through the constructor, it is the lack of cohesion implied by that constructor. The controller is doing too much. Just as a quick example - ICacheService, IEmailerService, and CryptoService. These should be handled by the infrastructure, *not* the controller. That alone eliminates three "services."http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment22http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment22Wed, 19 Oct 2011 13:41:25 GMTI thought that Email, Crypto and Cache Service could be static classeshttp://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment21http://ayende.com/123905/your-ctor-says-that-your-code-is-headache-inducing-introduction#comment21Wed, 19 Oct 2011 13:32:53 GMT