Presumably all of these constructor arguments are being injected. Usually in this kind of situation I'd create a runtime environment where all the services live, and can send messages amongst themselves if neccessary.

The amount of services being injected may indicate that a lot of bussiness logic is encoded into the controller.

Most of the business logic should be contained in a service / or just a few services.

I would understand if it was generating a report or export for something, but for an OrderController? Seems like there's so much stuff that is completely unrelated to the Order itself.

The screen should be composed rather than relying on a single controller to build the entire screen.

I'd bet that those services contain simple pieces of domain logic which really ought to be in the domain model (which I'm betting is anemic). The workflow is being directed by the controller, making it more like a transaction script...

Hope that IncentiveService doesn't do anything important...

I did notice a mix of ortogonal aspects (persistence, cache, crypto, notifications) that might be aggregated within an infrastructure facade.

From an initial glance it looks like the Fat Controller anti-pattern: http://codebetter.com/iancooper/2008/12/03/the-fat-controller/

That said, as Matt points out, the naming of these items would suggest an anemic domain model - Sales, Cart and Account _services_?

I guess there are no incentives to placing orders :)

The fact that those services are being injected does not justify the utter lack of cohesion. This controller is almost serving coffee.

I can see there might be issues there and I've tried to avoid just this thing in my code (too many injected dependencies). But it's hard to find a proper 'don't do it like that, do it like this' article. Everyone seems to just say, 'just don't do it'.

What I am after and what hopefully someone will point to is a good reference implementation of for example MVC 3 with a service layer and nicely done database layers (unit of work etc).

I thought the Nerd Dinner reference app would be good but it does not seem to have split things out into different logical areas. Does anyone have a link to the 'definitive' way to do this or is it all subjective?

@Jon Haha yeah, and lets hope the inevitable null-pointer exception isn't just caught and forgotten :)

This way for writing constructor make sure all required properties are set.

This way of constructor make sure all required properties are set - and caller has not missed anything.

It looks like the rest of the code is going to violate Single Responsibility Principle, and very likely it's going to be a fat controller (as Stuart says).

It's risky to tell without seen the rest of the code, but I'm sure this controller is going to have a lot of un-related methods (the fact that methods are working over the same set of entities is not a good excuse to include several responsibilities in the same controller).

@Ryan: google for Single Action Controllers. My own comments here: http://rodolfograve.blogspot.com/2011/05/teamcommons-mvc-single-responsibility.html and a working implementation here: http://nuget.org/List/Packages/TEAM.Commons.Web.

I agree with Ryan O'Neill, it is easy to say 'don't do it' and in best case throw in some oversimplified example of how should it be, but if you extend that sample to real world, it is becoming again 'don't do it'. As someone mentioned, you can hide all that dependencies with infrastructure facade, but that me reminds this video: http://www.youtube.com/watch?v=7RJmoCWx4cE

forgive me for being slightly off topic, but something you mentioned in this post sparked a personal question in me.

Ayende, would you feel it safe to say that programmers' push to adhere to 'best practices' has led to a point at which we frequently discard reasonable and intelligent solutions in favor of simply conforming to 'standards and practices'?

please take no offense - I respect you as one of the best programmers I have ever read code from, but I find that you disregard certain 'best practices' at times, and it works well for you. To cite a single example, nested classes are strongly discouraged by .NET 'best practices', and you have adopted a convention in your demos using nested classes on what I would personally call a frequent basis. And this seems to work very well for what it is demonstrated for.

do you think a lot of these practices had good merit for their original purposes, but when followed too strictly led to more problems - or do you think a lot of these practices were just outright wrong? Or, do you feel that most programmers just do not understand the practices? There are probably other answers I am neglecting.

Can be that most of this services are used in many pages of the site (and declared in ctor to be injected by Ioc container). The possible solution can be to create a single class

interface IServices{ public Iservice1 s1{get;set;} public Iservice2 s2{get;set;} }

class Services() : IServices{ Services(Iservice1 s1,Iservice2 s2....){ } Iservice1 s1{get;set;} Iservice2 s2{get;set;} }

then all the pages of the site that use this common service can have a more simple and maintenable ctor like this

public class OrderController : controller{ public IServices Services; public OrderController(IServices services){ Services = services; } }

Glad to see the review of this code base. I used it too for a refactoring/unit testing talk I did a few weeks ago for Typemock. The code is a huge mess, especially the "unit tests" that were already present in the project, 100 line "test" that has no beginning and no real end.

The Unity IoC solves the bloated ctor by adding the DependencyAttribute to the fields of the controller that you want Unity to resolve.

http://www.pnpguidance.net/Post/UnityIoCASPNETMVCFrameworkDependencyInjectionControllers.aspx

Everybody is talking about this being a violation of SRP, Fat Controller, etc... which is all well and good. However, I have feeling Ayende is going to have a different bone to pick with this code. We'll see....

I thought that Email, Crypto and Cache Service could be static classes

@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."

@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.

I foresee gallons of "this sucks, they are doing it wrong" and absolutely NO "instead they should do it this way" in this series.

@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.

Travis, 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.

At 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.

The 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.

By 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.

@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?

As 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.

I 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).

Naming a class "XxxService" is bad practice, its like all those manager classes.

I 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.

totally 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)

Tobi, MVC is a UI pattern. Controllers should not be responsible (directly) for sending email. That should be done in the infrastructure layer.

I'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. :~(

Justin - 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.

(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 blog

Hey 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

I 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.

Definitely 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..

Luc, 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?

I 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?

The following blog post from Mark Seemann describes how to handle such situations:

http://blog.ploeh.dk/2010/02/02/RefactoringToAggregateServices.aspx

Aren'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?

@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.

R

@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.

Hey 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.

@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.