Constructor Injection vs Setter Injection
Posted by Davy Brion on November 2nd, 2009
For those of you who’ve used Dependency Injection, you know that the two most common ways of injecting a dependency into a class are constructor injection and setter injection. For those of you who haven’t used Dependency Injection yet, here’s a simple example which shows both techniques:
public class MyService : IMyService
{
private IRequiredDependency requiredDependency;
public IOptionalDependency OptionalDependency { get; set; }
public MyService(IRequiredDependency requiredDependency)
{
this.requiredDependency = requiredDependency;
}
public void DoSomething()
{
// do something cool and/or important
// ...
}
}
This example is very abstract, but it should be pretty clear. Constructor injection is used to inject the required dependency whenever an instance of MyService is created, whereas setter injection is used to inject the optional dependency after the instance is created. I obviously can’t speak for everyone who uses Dependency Injection, but generally speaking most people use constructor injection for required dependencies and setter injection for optional dependencies.
There is however one situation in which i prefer setter injection for required dependencies over constructor injection: dependencies of abstract classes or base classes. For instance, in our service layer each incoming request is handled by a specific RequestHandler. Most of our RequestHandlers need our NHibernate infrastructure to be set up, which is automatically taken care of by our UnitOfWork implementation. So we have the following NhRequestHandler class (simplified for the purpose of this blog post):
public abstract class NhRequestHandler<TRequest, TResponse> : RequestHandler<TRequest, TResponse>
where TRequest : Request
where TResponse : Response
{
public IUnitOfWork UnitOfWork { get; set; }
public override Response Handle(Request request)
{
using (ITransaction transaction = UnitOfWork.CreateTransaction())
{
Response response;
try
{
response = base.Handle(request); // calls the specific Handle(TRequest) method of the derived handler
transaction.Commit();
}
catch (Exception e)
{
transaction.Rollback();
throw;
}
return response;
}
}
}
As you can see, the IUnitOfWork dependency is a required dependency because you would get a NullReferenceException when trying to handle a request without having a IUnitOfWork instance present. Yet, i really don’t want to put it in the constructor because then each and every RequestHandler that derives from this will also have to put it in the constructor, even though most of them won’t access the IUnitOfWork directly.
Actually, most of our applications inherit from the NhRequestHandler and then add some more dependencies that some kind of base BusinessRequestHandler will need. These are dependencies to deal with authentication, authorization, user context, application context, etc… Some of these dependencies will be used by the derived RequestHandlers, some won’t. All of them however will indeed be used by the BusinessRequestHandler so they are definitely required dependencies. Using constructor injection for these dependencies would lead to ‘noise’ in every derived RequestHandler’s constructor.
Instead, we use setter injection for all of a base-type’s dependencies, and use constructor injection only for the dependencies of the derived types. It keeps the constructors as clean as they can be and avoids unnecessary noise. We know that our IOC container will fulfill all the constructor dependencies as well as each property dependency in the inheritance hierarchy so there’s no chance of anything going wrong there. Unless of course somebody seriously breaks the IOC configuration but in that case, our applications won’t even make it through the simplest of requests so that’s not something that will ever happen unnoticed.
And for our tests, we always inherit our fixtures from things like HandlerTest or ControllerTest or whatever where all of those property dependencies are automatically set up with mocks or stubs, so it doesn’t really cause problems there either.
November 2nd, 2009 at 2:06 pm
I believe if you have too much ‘noise’ (too many dependencies) then you just violate SRP.
The class 99.9% should be split. Thus all dependencies will be split as well and make the code much cleaner.
Using propery injection to solve ‘noisiness problem’ is just akward workaround for the real issue – SRP violation.
Cheers.
November 2nd, 2009 at 2:10 pm
@Dmitriy
in most cases, i’d agree with that sentiment… but not in the context of the example given in the post
November 3rd, 2009 at 8:02 am
@Davy – Dmitriy is still right, though, even given your example. Heck, especially given your example. It would be trivial to refactor such a handler design so that individual handler implementations execute within a transaction and are provided a unit of work, yet perform none of the unit of work prep, transaction creation, commit, or even the rollback in case of a failure. Since the ability to perform such a refactoring might be dependent on the simplicity of your provided example, can you provide a more real-world example we can look at?
November 3rd, 2009 at 9:23 am
@Jeremy
that actually is a real-world example
how would you change the design so that the handler executes within the scope of the UnitOfWork _and_ has access to it? I could’ve gone the AOP route (either dynamically with Windsor or statically with Postsharp) but then i wouldn’t have access to the UOW in the derived handlers. If you have anything else in mind, please do share
Also, classes that derive from this specific handler typically introduce another dependency for context-related data so it can easily be accessed in the handlers that derive from that.
I really don’t see this is as a violation of the Single Responsibility Principle.
November 3rd, 2009 at 5:26 pm
I have only a few moments to reply before I must head off to the office, but off the top of my head the main option jumping out at me is move your Template Method implementation away from inheritance and towards composition (via nested closure or the like). You can do this in a few steps to make it easier.
The first step is to change the signature for your template method body from base.Handle(request) to base.Handle(UnitOfWork, request), allowing you to remove the derived type dependencies on the UnitOfWork property. Next, just to enforce that change, take the getter off the UnitOfWork property, or at least make it private to the base type. Then, replace inheritance with composition. Instead of deriving to implement Handle(UnitOfWork, TRequest) you instantiate just one implementation of a request handler and then call Handle. Handle would do your familiar transaction setup and then call a closure of the form TResponse Handle(IUnitOfWork, TRequest). The closure itself would assume operation within a transaction that commits on successful return and rolls back on any exception thrown from the closure. The closure would have been passed into the call to Handle itself, assuming the Handle signature can be modified, or passed into the ctor (if you are using a container that supports dependency override (StructureMap does, for example)) if the Handle signature cannot be modified. If it cannot, the ctor injection would do nicely. If your container can’t do dependency overrides, either get one that can or use setter injection for the closure.
Now, all of that said, if you ended up back at setter injection of a closure instead of setter injection of the unit of work, one would have to consider if the improvement of killing the SRP violation was worth the trouble. With that in mind, much of the above hinges itself on whether or not you can change the signature of the Handle method to accept the closure or your container (and instantiation pattern) will allow you to use ctor dependency override.
As for “I really don’t see this is as a violation of the Single Responsibility Principle.”, addressing unit of work and transactions is a separate concern from request-specific handling logic, and your current use of Template Method pattern simply proves the existence of the SRP violation, as much as it is occluded behind the responsibilities of the base type versus those of the derived types. That said, your current approach certainly isn’t a huge SRP violation and there may well be more pressing matters to hunt down within your codebase since your application of Template Method does a good job of minimizing the visibility and impact.
You could for the moment just do part of the change, as a prep move and a valuable piece of clean-up in its own right: the first two steps, changing the base.Handle(request) signature to also accept the unit of work and then hiding the UnitOfWork property from the derived types (and from outside code, which has its own benefit.)
November 3rd, 2009 at 6:10 pm
By the way, I have some other variations on the implementation eluded to above, so just let me know if you’d like to hear about them. They might align better with your implementation constraints, worldview, etc.
November 4th, 2009 at 8:37 am
@Jeremy
i’m still not convinced of the approach you mentioned in your comment. The end result would be practically the same, and i wouldn’t really get any benefit out of it (and it would hurt backwards compatibility which i have to take into account). The reason why i don’t really get any benefit out it is because nobody ever has a reference to a handler. A specific handler is instantiated by the IOC container whenever a request comes into the service layer, and the Handle(Request) method is called, after which the resulting Response is just returned. You never instantiate or deal with a handler yourself, so the fact that the IUnitOfWork instance is exposed by a public getter/setter doesn’t really hurt anyone. Theoretically it certainly doesn’t have to be there and it shouldn’t be exposed. But if nobody gets the chance to actually access it (apart from the IOC container that provides the dependency), is there really a problem?
As for the SRP thing… i guess it depends on how strict you are about it. For me, a specific handler class will only have code to take care of the functionality it needs to provide. Nothing more, nothing less. It doesn’t need to worry about the UnitOfWork or the database transaction and automatic commits/rollbacks. Whether that is achieved through inheritance or composition doesn’t really matter to me so long as there is no noticeable downside to it.
that said, i do enjoy these sort of discussions so please feel free to share your other variations
November 4th, 2009 at 11:10 am
@Jeremy
The solution you provided, doesn’t seem like a very good solution to me.
By doing this, the RequestProcessor (the one who resolves the RequestHandlers and calls their Handle method) would have to know which types have a Handle method with a UnitOfWork and which don’t, probably resulting in an if-cast and at least 2 different ways to call a RequestHandler. If for some reason another type of RequestHandler is introduced it would result in yet another if-cast-callTheSpecificHandleMethod.
Or am I missing something and how would the RequestProcessor look like then?
November 13th, 2009 at 3:51 pm
Late to the conversation (I just discovered the blog through Elegant Coders), sorry.
I am not seeing a violation of SRP here. The NhRequestHandler’s job is to wrap the response in a transaction. It has only one reason to change – the nature of handling a transaction. If anything, the name is strange because it says Nh, I would rename the class to TransactionalRequestHandler. The only other thing I might do is expose a method on IUnitOfWork called WithinTransaction(Action work) as I am sure you have the try..commit…catch…rollback a lot in the code base and it would remove some of the “noise” that bothers others.
I would, however, consider changing the RequestHandler so that you don’t have to call back into the base class’s Handle method to properly finish handling the request. It justs seem strange to me and counter-intuitive. I have run into this a bit in my Test infrastructure and I can never remember if I have to call back into my base class or not (think test fixture setup). I think that would lead me to using the Decorator pattern here, because I am not sure inheritance is correct in this case – probably a huge undertaking that’s not worth it at this point.
The reason I say Decorator, is because you are really wrapping the actual response handling code inside of a transaction. Your NhRequestHandler really does nothing else. I think it would be more intuitive to see RequestHandlerDecorator(RequestHandler).Handle and then your code would be more intuitive.
public Response Handle(Request request)
{
UnitOfWork.WithinTransaction(() =>
handler.Handle(request);
);
}
November 13th, 2009 at 3:59 pm
@Brian
that try/commit/catch/rollback block is actually the only one we have, which is why i don’t bother with cleaning it up
November 13th, 2009 at 4:06 pm
@Davy – kudos to you and YAGNI to me
January 27th, 2010 at 9:05 pm
[...] In case you’re wondering why i’m using Setter-injection here instead of Constructor-injection, read this. [...]