Reputation: 35216
I don't like constructor based dependency injection.
I believe that it increases code complexity and decreases maintainability, and I'd like to know if there are any viable alternatives.
I'm not talking about the concept of separating implementation from interface, and having a way of dynamically resolving (recursively) a set of a objects from an interface. I completely support that. However, the traditional constructor based way of doing this seems to have a few issues.
1) All tests depend on the constructors.
After using DI extensively in an MVC 3 C# project over the last year, I find our code full of things like this:
public interface IMyClass {
...
}
public class MyClass : IMyClass {
public MyClass(ILogService log, IDataService data, IBlahService blah) {
_log = log;
_blah = blah;
_data = data;
}
...
}
Problem: If I need another service in my implementation I have to modify the constructor; and that means that all of the unit tests for this class break.
Even tests which have nothing to do with the new functionality require at least refactoring to add additional parameters and injecting a mock in for that argument.
This seems like a small issue, and automated tools like resharper help, but its certainly annoying when a simple change like this causes 100+ tests to break. Practically speaking I've seen people doing dumb things to avoid changing the constructor rather than bite the bullet and fix all the tests when this happens.
2) Service instances get passed around unnecessarily, increasing code complexity.
public class MyWorker {
public MyWorker(int x, IMyClass service, ILogService logs) {
...
}
}
Creating an instance of this class if only possible if I am inside a context where the given services are available and have automatically been resolved (eg. controller) or, unhappily, by passing the service instance down multiple chains of helper classes.
I see code like this all the time:
public class BlahHelper {
// Keep so we can create objects later
var _service = null;
public BlahHelper(IMyClass service) {
_service = service;
}
public void DoSomething() {
var worker = new SpecialPurposeWorker("flag", 100, service);
var status = worker.DoSomethingElse();
...
}
}
In case the example is not clear, what I'm talking about is passing resolved DI interface instances down through multiple layers from no reason other than at the bottom layer they are needed to inject into something.
If a class does not depend on a service, it should be have a dependency on that service. This idea that there is a 'transient' dependency where a class doesn't use a service but simply passes it on is, in my opinion, nonsense.
However, I don't know of a better solution.
Is there anything that provides the benefits of DI without these problems?
I've contemplated using the DI framework inside the constructors instead as this resolves a couple of the problems:
public MyClass() {
_log = Register.Get().Resolve<ILogService>();
_blah = Register.Get().Resolve<IBlahService>();
_data = Register.Get().Resolve<IDataService>();
}
Is there any downside to doing this?
It means that the unit tests must have 'prior knowledge' of the class to bind mocks for the correct types during test init, but I can't see any other downsides.
NB. My examples are in c#, but I've stumbled into the same issues in other languages too, and especially languages with less mature tooling support these are major headaches.
Upvotes: 47
Views: 16175
Reputation: 655
So I have heard of a coding pattern before where you create a Context object that gets passed around everywhere throughout your code. The Context object houses any "injections" and/or services you would like to control about your environment.
It has a general feel of GVs, but you have a lot more control over them, because you can default them to null for anything outside the package, so you can have tests within the package that can access the protected constructer, thus allowing you to control the Context object.
So for example main class:
public class Main {
public static void main(String[] args) {
// Making main Object-oriented
Main mainRunner = new Main(null);
mainRunner.mainRunner(args);
}
private final Context context;
// This is an OO alternative approach to Java's main class.
protected Main(String config) {
context = new Context();
// Set all context here.
if (config != null || "".equals(config)) {
Gson gson = new Gson();
SomeServiceInterface service = gson.fromJson(config, SomeService.class);
context.someService = service;
}
}
public void mainRunner(String[] args) {
ServiceManager manager = new ServiceManager(context);
/**
* This service be a mock/fake service, could be a real service. Depends on how
* the context was setup.
*/
SomeServiceInterface service = manager.getSomeService();
}
}
Example test class:
public class MainTest {
@Test
public void testMainRunner() {
System.out.println("mainRunner");
String[] args = null;
Main instance = new Main("{... json object for mocking ...}");
instance.mainRunner(args);
}
}
Note, it is a bit of extra work, so I would probably only use it for microservices and small apps. Large apps would be easier to just do Dependency Injection.
Upvotes: 2
Reputation: 2545
In my opinion, the root cause of all the problems is not doing DI right. The main purpose of using constructor DI is to clearly state all the dependencies of some class. If something depends on something, you always have two choices: making this dependency explicit or hiding it in some mechanism (this way tends to bring more headaches than profits).
Let go through your statements:
All tests depend on the constructors.
[snip]
Problem: If I need another service in my implementation I have to modify the constructor; and that means that all of the unit tests for this class break.
Making a class depend on some other service is a rather major change. If you have several services implementing the same functionality I would argue that there is a design issue. Correct mocking and making tests satisfy SRP (which in terms of unit tests boils down to "Write a separate test per each test case") and independent should resolve this problem.
2) Service instances get passed around unnecessarily, increasing code complexity.
One of the most general uses of the DI is to separate object creation from business logic. In you case we see that what you really need is to create some Worker which in turn needs several dependencies which are injected through the whole object graph. The best approach to resolve this issues is to never do any new
s in the business logic. For such cases I would prefer to inject a worker factory abstracting business code from actual creation of the worker.
I've contemplated using the DI framework inside the constructors instead as this resolves a couple of the problems:
public MyClass() { _log = Register.Get().Resolve<ILogService>(); _blah = Register.Get().Resolve<IBlahService>(); _data = Register.Get().Resolve<IDataService>(); }
Is there any downside to doing this?
As a benefit you will get all the downsides of using the Singleton
pattern (untestable code and a huge state space of your application).
So I would say that DI should be done right (as any other tool). The solution to your problems (IMO) lies in understanding DI and education of your team members.
Upvotes: 22
Reputation: 233487
It's tempting to fault Constructor Injection for these issues, but they are actually symptoms of improper implementation more than disadvantages of Constructor Injection.
Let's look at each apparent problem individually.
All tests depend on the constructors.
The problem here is really that the unit tests are tightly coupled to the constructors. This can often be remedied with a simple SUT Factory - a concept which can be extended into an Auto-mocking Container.
In any case when using Constructor Injection, constructors should be simple so there's no reason to test them directly. They are implementation details which occur as side-effects of the behavioral tests you write.
Service instances get passed around unnecessarily, increasing code complexity.
Agreed, this is certainly a code smell, but again, the smell is in the implementation. Constructor Injection can't be blamed for this.
When this happens it's a symptom that a Facade Service is missing. Instead of doing this:
public class BlahHelper {
// Keep so we can create objects later
var _service = null;
public BlahHelper(IMyClass service) {
_service = service;
}
public void DoSomething() {
var worker = new SpecialPurposeWorker("flag", 100, service);
var status = worker.DoSomethingElse();
// ...
}
}
do this:
public class BlahHelper {
private readonly ISpecialPurposeWorkerFactory factory;
public BlahHelper(ISpecialPurposeWorkerFactory factory) {
this.factory = factory;
}
public void DoSomething() {
var worker = this.factory.Create("flag", 100);
var status = worker.DoSomethingElse();
// ...
}
}
Regarding the proposed solution
The solution proposed is a Service Locator, and it has only disadvantages and no benefits.
Upvotes: 17