Dependency injection in legacy code

Question

I have a legacy project with many classes with high coupling, and I have a necessity to test some of them. So, I decided to introduce dependency injection principle but stumbled upon where to create dependencies.

class Context
{
    ICallManager* m_pCallManager;
public:
    Context(ICallManager* pCallManager);
...
};

There has to be a place where I create CallManager like this

ICallManager callManager = new CallManager();

I read about composition root, but I don't like this approach because it breaks encapsulation and there are too many classes and the hierarchy is to deep to inject everything.

So far I am thinking of two solutions:

1. To leave some members as is and rely on unit tests of that member's class. Doesn't work well for 3rd party libs/timers, etc.

class Context
{
    CallManager* m_pCallManager;
public:
    Context()
    {
        m_pCallManager = new CallManager();
    }
    // We assume that CallManager doesn't have bugs
};

2. To use some kind of factory with enum for types. It's like composition root, but I don't create all objects and just give enum variable which type to use.

enum class Types
{
    PROD = 1,
    TEST = 2
};

struct Factory
{
    static ICallManager createCallManager(Types type)
    {
        switch(type)
        {
        case PROD: return new CallManager(); break;
        case TEST: return new CallManagerMock(); break;
        }
    }
};

class Context
{
    ICallManager* m_pCallManager;
public:
    Context(Types type)
    {
        m_pCallManager = Factory::createCallManager(type);
    }
...
};

void main()
{
    Context c = new Context(Types::PROD);
}

Is there a good compromise between encapsulation, testability and maintainability?

Answer 1

I have to be honest, I was slightly triggered by your last sentence:

Is there a good compromise between incapsulation, testability and maintainability?

This sentence to me, says that the three are opposites of each other, which I do not in any way think that they are. Rather I would say that testability and maintainability are closely related, and so are encapsulation and maintainability and even maintainability and encapsulation. So choosing one, should not be at the expense of the others, rather the opposite.

That comment aside I am a huge advocate of simple constructor injection. It has the distinct advantage of being easy to explain, and easy to test. Further more it clearly visualizes the dependencies of a class. If you end up with too many constructor parameters there is a fair chance that your class is violating the Single Responsibility Principle.

We do not want to include parameters in our constructor that we do not directly use. Classes are instantiated where it makes logical sense, for some that will be in the root of your program, for others it will be in producer like classes.

For instance a class might read a file and produce an instantiation of a class as a result. Then the instantiation of that class will be made insight of the producer.

I think logically that makes the code easier to analyze, it makes the code more testable, since mocks or test instances can be passed directly to the constructor of the class under test. Also it makes the code more maintainable since refactoring instance creation can be changed to a few places.

I am not a fan of the enum approach since it creates a branch in your code that is separate between production and test. I like my tests to take the same paths through the code as my production. Instead you should instantiate mocks that is passed to the constructor, and thereby isolate the tests, to as small a segment of your code as possible (when testing on a unit level).

In my personal experience, I try to design for constructor injection when writing new code (actually being rather strict about it), and then applying an injection framework later on if I decide to. And favor injection frameworks more frequently when working on legacy systems. I do not have the greatest experience with injection libraries in C++, but I would think they would be similar to the ones in Java: Dagger, Guice, etc.

Answer 2

Warning: I haven't written C++ code in 20 years....

But back in my day, we would have considered constructor overloading

public:
    // Our unit tests invoke this constructor, passing
    // an instance of an ICallManager controlled by the
    // test, for example CallManagerMock
    Context(ICallManager* pCallManager)
    {
        m_pCallManager = pCallManager;
    }

    // This code would not be covered by tests that use
    // a substitute implementation of ICallManager; but
    // that's OK because this implementation is "so
    // simple that there are obviously no deficiencies."
    Context()
    : Context(new CallManager())
    {
    }

---

To use some kind of factory with enum for types. It's like composition root, but I don't create all objects and just give enum variable which type to use.

You could do that, but it isn't clear to me that it is a big win; consider:

struct Factory
{
    static ICallManager createLiveCallManager();

    static ICallManager createInertCallManager();

    static ICallManager createCallManager(Types type)
    {
        switch(type)
        {
        case PROD: return createLiveCallManager();
        case TEST: return createInsertCallManager();
        }
    }
};

Riddle: is the switch statement, and the specification of Type, really buying you anything vs just directly calling the function that you need?

(In some contexts, the answer is "yes, it does"; I'm not arguing that it is always wrong to switch like this -- just that you should be able to articulate the clear specific benefits it brings to your context.)

Answer 3

You should look into Dependency Injection libraries. The library acts as a parametrable catalog of type resolution strategies, and will handle the complexity of the type graph dependcy resolution. You can use different DI configurations depending on the entrypoint, environment (dev/prod), or feature flags.

Example with boost-di:

int main() {
  const auto injector = di::make_injector(
    di::bind<interface>.to<implementation>()
  );

  injector.create<std::unique_ptr<example>>();
}

Some dependency injection library provide additional features such as lifetime managers, allowing you to configure when a type resolution should create a new instance or reuse an existing one.

I don't know much about C++ ecosystem in that aspect, but most language also have mock-DI libraries. These libraries will configure the DI library to automatically try to resolve interfaces with mocked objects unless the interface type is explicitly bound to an implementation. This makes writing of unit tests extremely simple while using the exact production code as the code under test.

Answer 4

At the moment you have a badly designed singleton. Not just a singleton, but one that cannot be used easily. Change that first. Your "Context" gets a public method that returns the same Context instance on every call, set up properly. That method is the only one creating a Context instance. The code to create a Context instance is private. You decide where it gets its type from, but not from a caller. You call the class method to get a context every time when you need it and at no other times.

Now you have moved up to a well designed singleton. And now you look at all the instances calling that singleton, and each constructor for such an instance gets the singleton passed in instead (that's the "dependency injection"). That singleton is stored, and you use the stored instance instead of asking the Context class for one.

Now the caller of your constructor will use the Singleton. You do the same for the constructor, until only one place is left that uses the Singleton. And that one place would have to be modified for example for unit tests, if you want a different dependency.