Is this design for dependency injection easy to understand?

Question

I'm working on a project with a large existing codebase and I've been tasked with performing some re-engineering of a small portion of it. The existing codebase does not have a lot of unit testing, but I would like at least the portion I'm working on to have a full suite of unit tests with good code coverage.

Because the existing codebase was not designing with unit testing in mind, I need to make some structural changes in order to support isolation of dependencies. I'm trying to keep the impact on existing code as small as possible, but I do have a bit of leeway to make changes to other modules. I'd like feedback on whether the changes I've made make sense or if there is a better way to approach the problem. I made the following changes:

1. I extracted interfaces for all of the non-trivial classes on which my 'coordinator' class depends. The other developers grudgingly allowed this, with some minor grumbling.

2. I modified the coordinator class to refer only to the interfaces for its operation.

3. I could not use a DI framework, but I had to inject a large number of dependencies (including additional dependency instantiation as part of the class operation), so I modified the coordinator class to look like this (domain-specific names simplified):

.

public sealed class Coordinator
{
    private IFactory _factory;

    public Coordinator(int param)
        : this(param, new StandardFactory())
    {            
    }

    public Coordinator(int param, IFactory factory)
    {
        _factory = factory;
        //Factory used here and elsewhere...
    }

    public interface IFactory
    {
        IClassA BuildClassA();

        IClassB BuildClassB(string param);

        IClassC BuildClassC();
    }

    private sealed class StandardFactory : IFactory
    {
        public IClassA BuildClassA()
        {
            return new ClassA();   
        }

        public IClassB BuildClassB(string param)
        {
            return new ClassB(param);
        }

        public IClassC BuildClassC()
        {
            return new ClassC();
        }
    }
}

This allows for injection of a stubbed factory to return the mocked dependencies for unit testing. My concern is that this also means any user of this class could supply their own factory to change the way the class operates, which is not really the intention. This constructor is purely intended for unit testing, but it has the side effect of implying that additional extensibility was intended. I don't usually see this pattern in other classes I use, which makes me wonder if I'm over-complicating and if there's a better way to achieve the required isolation.

---

NOTE: There's something weird going on with the code formatting, so I apologize for the poor formatting above. Not sure why it won't let me format it correctly.

Answer 1

You might want to consider making the constructor that accepts an IFactory internal -- that would prevent other users outside the assembly from "extending" your Coodinator. Then use InternalsVisibleTo attribute to expose it only to your unit tests.

Answer 2

The proposed design is a variant of Constructor Injeciton sometimes known as Bastard Injection. In a greenfield situation I would consider Bastard Injection an anti-pattern, but it's a good compromise in a brownfield situation like the one you describe.

You shouldn't really be concerned about the implied extensibility of the Coordinator class. Done correctly, testability is extensibility, so ideally the injected IFactory should represent a proper Domain Concept that enables variability. I understand that this can be difficult/impossible to achieve at a single stroke when you're refactoring from legacy code, but that's the direction in which your code base should be moving.

In any case, if your coworkers already begrudge the extraction of interfaces, they are probably not very likely to interpret the IFactory constructor parameter as an extensibility point.

Good luck.