Ayende @ Rahien

It's a girl

Your ctor says that your code is headache inducing: Explanation

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:

image_thumb

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:

image

And yes, this does give me a headache, too.

Comments

Khalid Abuhakmeh
10/20/2011 10:55 AM by
Khalid Abuhakmeh

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.

Paulo Aboim Pinto
10/20/2011 11:02 AM by
Paulo Aboim Pinto

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

Mike
10/20/2011 11:26 AM by
Mike

Me too! Me too! Ayende, how do you resolve/refactor such problems? Greetings from Cyprus!

Luc
10/20/2011 11:49 AM by
Luc

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

Matthew Shapiro
10/20/2011 12:10 PM by
Matthew Shapiro

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.

Rodolfo Grave
10/20/2011 12:14 PM by
Rodolfo Grave

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

John
10/20/2011 12:32 PM by
John

Hey, thanks for the suggestions on how to improve the code! Great Post!

....sheesh.....

Tudor
10/20/2011 12:33 PM by
Tudor

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.

tobi
10/20/2011 12:58 PM by
tobi

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.

Glen
10/20/2011 02:08 PM by
Glen

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!

Rodolfo Grave
10/20/2011 03:43 PM by
Rodolfo Grave

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

Jim Danby
10/20/2011 04:21 PM by
Jim Danby

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.

Straw Man
10/20/2011 05:51 PM by
Straw Man

Typical Ayende Straw Man. Seems like everyone is starting to catch on to this guy. How boring Ayende. Dont waste our time anymore.

tobi
10/20/2011 06:00 PM by
tobi

@Jim Danby: The set of depencies being big is not a problem if default implementations will be sufficient.

Jon
10/20/2011 09:57 PM by
Jon

Yo, this doesn't look like the documentation for Rhino Mocks. Yer link is broken here: /blog/592/updated-rhino-mocks-documentation

Scooletz
10/21/2011 09:09 AM by
Scooletz

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 atleast_ their names refactored

tobi
10/21/2011 10:34 AM by
tobi

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.

Scooletz
10/21/2011 11:25 AM by
Scooletz

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

JoeSmith
10/21/2011 03:09 PM by
JoeSmith

So can someone explain this further, maybe show a refactored example. I struggle with this concept.

John Farrell
10/21/2011 05:55 PM by
John Farrell

Love these posts, hate the constant comments from developers who want hand holding.

Bob
10/22/2011 03:44 AM by
Bob

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.

Straw Man
10/24/2011 01:11 AM by
Straw Man

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 Fanboi
10/25/2011 01:03 AM by
Straw Man Fanboi

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

JayD
10/25/2011 01:42 AM by
JayD

@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
10/26/2011 12:21 PM by
JayD

@JayD hahaha that's a good one.

DannyA
10/26/2011 06:06 PM by
DannyA

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.

Jospeh
11/08/2011 11:47 PM by
Jospeh

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.

Comments have been closed on this topic.