Sunday, February 5, 2012

Demonstrating the Costs of DI

Here's a followup to my previous post "Interfaces, DI, and IoC are the Devil"

This time I want to demonstrate a little of what I'm talking about with some code samples.  I have contrived an example.  I wanted something that is real, so I took the code structure and the responsibilities from some real code in one of our applications.  But I changed the names to make it more easily understood.  I also removed some of the concerns, such as transaction handling and record locking, just to make it shorter.  I'm trying to be fair, so the example is not trivial, but it is also not complicated.

Note that I didn't compile any of this code, so please excuse typos or obvious syntactic errors I might have over looked.

Pretend we are writing an application to manage a conference.  It's an MVC 3 C# web app.  We have presentations, and presentations have speakers.  The speakers are ordered (1,2,3, etc).  As an optimization, we will cache some of the information about the speakers for a given presentation: how many are there and who is #1 ordered speaker.  This information will be cached on the PresentationSpeakers table.

The structure of the code is as follows.  The SpeakerController's Create action method is called to add a new speaker to a presentation.  This controller delegates the job to the PresentationApi class.  This class deals with coordinating the various domain services, and in real life would have dealt with database transactions and record locking/concurrency.  PresentationApi delegates to OrdersSpeakers (which obviously assigns the order numbers to the speakers) and PresentationSpeakers (which caches information about the speakers on a presentation).  Finally the Speaker and Presentation classes are active record objects.

This first example demonstrates the simplest way in which I would like to write this code.  There are no interfaces, there is no constructor injection, and it is using the Active Record pattern for database persistence.

The next example addresses the "problem" of the controller having a direct dependency on the PresentationApi class by adding IPresentationApi and injecting it through the controller's constructor. Notice that I also convert PresentationApi to be a singleton at this point and remove it's instance variables. This isn't strictly required, but it is typical. Notice how I now have to pass the presentationId into the LoadPresentationSpeakersRecord helper method.

In the third example, I remove PresentationApi's direct dependency on OrdersSpeakers.

Finally I eliminate ActiveRecord and replace it with the most evil pattern in the world, the Repository pattern. I chose to implement this in the way I've most commonly seen, with one interface per database table (give or take).

So, if you had your choice, which of these versions would you prefer to have in your code base? Which would you prefer to have to debug, understand, and maintain?

My answer, not surprisingly, is #1. Notice how each version adds more and more cruft into the code and obscurs what the code is actually trying to accomplish.  This is the cost of indirection.  It's really the same reason people like to fly the YAGNI flag.  And perhaps you've heard the old adage, "Do the simplest thing that could possibly work".  I desperately want as few constructs between me and what my code does as possible.  Which is why I yearn for the simple code in example #1.

PS.  If you actually read those code samples and studied the differences between them, pat yourself on the back!  I know this is very difficult to follow.  I actually wasn't intending to blog it, I just wanted to go through the motions myself and see how it turned out.  There have been quite a few times where I wrote something off because of my image of how it would turn out.  But then when I actually did it, it turned out much differently (MVC, MVP, and MVVM were all like that for me).  But in this case, it turned out just how I'd imagined it...  Crufty.

3 comments:

  1. I would prefer to have my code as close to #1 as possible as well. Unfortunately I think you're screwed in a static language no matter how you cut it. You basically have three options to try to eliminate the cruft:

    #1 - Have a default parameterless constructor that news up sensible defaults for objects it depends on.

    #2 - Use property injection instead: Func = () => new OrdersSpeakers();

    #3 - Leverage an IOC and never call new :(

    The only way to eliminate the interfaces would be to make all your public methods and properties virtual. :(

    ReplyDelete
  2. Interesting read. There's been a lot of backlash over the last year in particular against some of the bigger OOP tools (IoC, ORM, etc). For small applications, there is no justification for adding the complexity associated. But in the simple version, you're making a static call into an API that presumably hits a database. In a small application, you can just use seed data and integration tests. If this were a large application, or that API was a lot more complicated, I think you'd start looking for ways to break that coupling.

    I guess what I'm saying is, all technologies can suffer from "when all I have is a hammer...". And IoC is often put into apps prematurely. But like all tools, there is a valid reason for it. Sometimes it fits. My 2 cents...

    ReplyDelete
  3. Hey Tim, I was wondering if anyone would mention the static active record calls. I decided not to mention it in this post, but the AR framework I'm using here actually allows me to mock out the database. So while the code is coupled to a concrete implementation of the Save method, it is not coupled to the database!

    ReplyDelete

Note: Only a member of this blog may post a comment.