+972 52-548-6969
Your ctor says that your code is headache inducingExplanation
I was pointed to this codebase, as a good candidate for review. As usual, I have no contact with the project owners, and I am merely using the code as a good way to discuss architecture and patterns.
It starts with this class:
Okay, this codebase is going to have the following problems:
- Complex and complicated
- Hard to maintain
- Hard to test
- Probably contains a lot of code “best practices” that are going to cause a lot of pain
And I said that this is the case without looking at any part of the code except for this constructor. How am I so certain of that?
Put simply, with 9 dependencies, and especially with those kind of dependencies, I can pretty much ensure that this class already violate the Single Responsibility Principle. It is just doing too much, too complex and too fragile.
I shudder to think what is involved in testing something like that. Now, to be fair, I looked at the rest of the codebase, and it seems like I caught it in a state of flux, with a lot of stuff still not implemented.
Nevertheless… this is a recipe for disaster, and I should know, I have gone ctor happy more than once, and I learned from it.
And here is the obligatory self reference:
And yes, this does give me a headache, too.
More posts in "Your ctor says that your code is headache inducing" series:
- (20 Oct 2011) Explanation
- (19 Oct 2011) Introduction
Comments
I'd like to see how you would begin to refactor something like this. I use a lot of constructor injection as well (maybe not as extreme as above) but Sometimes it does get a little hairy.
Interested to see how you dissect and improve an existing code base.
Yes, I don't have this kind of code, but I use lot's of constructor injection. Because of that I use Dependency Injection module like MEF ou Unity to not hurry about this.
You raise a interested problem and I would like to know how you resolve this to be cleaner and testable code.
regards Paulo Aboim Pinto Odivelas - Portugal
Me too! Me too! Ayende, how do you resolve/refactor such problems? Greetings from Cyprus!
"Put simply, with 9 dependencies, and especially with those kind of dependencies,"
Agreed that it isn't just the nr of dependencies you have but also the way how they differ from each other.
I wonder how people respond to you when you criticize their codebase so publicly (=> not meant in an offensive way and with all respect to the authors of the code).
I personally refactor this by splitting the functionality into multiple controllers/classes. I've been burned by this (and trying to come up with "clever" solutions around it, that I realized just splitting up my functionality into more logical classes makes things much simpler and more manageable.
First step to avoid or refactor out of this kind of code, is to forget about the general idea that you must have one controller per "entity". After you break with that idea, you can start thinking about other rules to create smaller controllers with less responsibilities (ideally only one). The extreme application of that idea leads you to one single action in every controller.
Those controllers are small, easy to test, easy to read. Look for Single Action Controllers out there.
With some small tweaks to the ASP.NET MVC framework you can get a very nice application structure. For an example on how to do it, check http://rodolfograve.blogspot.com/2011/05/teamcommons-mvc-single-responsibility.html
Hey, thanks for the suggestions on how to improve the code! Great Post!
....sheesh.....
A single action in every controller might seem a nice idea, but it almost brings us back to procedural programming (classes with a single public method and without state are just that).. Single responsibility principle does not mean that each class should do a single thing, but more that each class should have a single reason to change.
The way you test this stuff is have your test use your IOC container to instantiate the class under test. Prepopulate the container with default/null implementations and only override what is necessary. For example, 100% of your tests will be happy to use a NullEmailService. Problem solved.
Rodolfo thats an interesting link - ill have a nose at that.
This issue is something that im coming accross now and it does cause some headaches!
@Tudor: not really. A single action in every controller means each request to the application is going to be treated as an event and the responsibility of the controller class is therefore defined as "react to event ...". This resembles the kind of code you get when you use an event driven architecture with a ServiceBus, and I can say that's a very nice thing to work with.
Think of your controllers as you Application Services and everything should fit.
Note that having a single action doesn't mean you can only have one method
@Glen I've been there, done that. The provided implementation in TEAM.Commons is usable, but can (and must) be improved, so don't evaluate the idea solely based on this implementation, but rather on the principle itself.
If you'd like more info, let me know and we can talk privately or I could even make a more ellaborated post on the subject.
I'd like to understand how you would refactor this. Just pointing and complaining isn't helpful. And using an IoC container isn't a solution either if you still have the same set of dependencies.
Typical Ayende Straw Man. Seems like everyone is starting to catch on to this guy. How boring Ayende. Dont waste our time anymore.
@Jim Danby: The set of depencies being big is not a problem if default implementations will be sufficient.
Yo, this doesn't look like the documentation for Rhino Mocks. Yer link is broken here: /blog/592/updated-rhino-mocks-documentation
Instead of fattening up the ctor, move the required interfaces to the methods, leaving only the common in the ctor. Then provide a model binder, which will use a container, to resolve parameters. Having that said, each method has all it needs and the controller itself is given only a common part of dependencies.
Having that many so-called services is a bad desing by itself. The majority is infrastructural stuff and should have at_least their names refactored
Scooletz, I am doing that in projects. It is a good technique but you really need to have a common set of services injected into the class. It gets very cumbersome without this.
@Tobi, Glad you do the same:] I do not consider it as a fix for a bad design, but rather a way to not be flooded with ctors like this. Making a programmer to add one parameter and field to be used for only one method - that is _wrong_.
So can someone explain this further, maybe show a refactored example. I struggle with this concept.
Love these posts, hate the constant comments from developers who want hand holding.
Another typical post, say something's shit and do not provide any useful feedback on why and how to fix it.
And yet again the fanbois will lap it up.
Ahhhh - John Farrell you would be one of the sycophant fanbois. I figured you'd need me to point that out to you.
@Straw Man
Agreed. As usual, he bashes the work of others without bothering to understand why they did what they did or offer up a better way to accomplish that task.
This guy is a knob.
@John Farrell It's easy to know when something tastes like shit but it's hard to be 5 star chef. Just pointing out that the code is shit isn't helpful--- most developers worth anything KNOW there is a problem but don't know how to fix it.
@JayD hahaha that's a good one.
There's no way this controller needs 9 services to be functionally complete. In my opinion, it all depends on how the workflows of this system are structure. Maybe placing the order, paying for the order and fulfilling the order are 3 distinct responsibilities and the controllers can be broken up that way.
I like the SRP post by Rodolfo and without really knowing how all these services are being used I can see some distinctions (SRPs) based on the dependencies.
1) An order controller 3) An order payment controller that takes cryptoservice, accountService, addressService etc.. 2) An order fulfillment controller that depends on EmailService, AddressService (maybe) etc...
My 2 cents....Hope it helps someone.
DannyA.
Agree with DannyA, instead of creating separate controllers though, these services could be wrapped up into Service Facades.
Have a look at Seemans Dependency Injection in .NET, chapter 6 is all about DI Refactoring with section devoted to Dealing with Constructor Over-injection.
Anyone wanting tips on how to refactor DI should read that chapter.
Comment preview