Avoid class chains that emerge from DI

Question

I am developing a hobby project where I try to use DI to get testable code. Until now, I found that it improved both the readability, usability, and testability of the code. However, now I have a situtation where I find that the usability suffers a lot and I think there is some key part of DI that I am missing.

In the initial setup phase of my application, I need to load a project file. In the Application class, the code is quite simple and readable:

void Application::run(int argc, char** argv)
{
    m_project.load(argc, argv);
    // other stuff following, e.g., run main loop
}

Its constructor receives a ICmdLineProject interface with a void load(int argc, char** argv) function. The concrete implementation now reads a file name from the command line arguments, parses the file, and then applies the stored values:

void FooProject::load(int argc, char** argv)
{
    std::string file_name = read_file_name_from_cmd_line(argc, argv);
    std::ifstream file(file_name);
    IniConfig ini(file);
    FooConfig config = parse_ini_config(ini);
    m_window_system.create_window(config.m_width, config.m_height);
}

When implementing the unit tests for FooProject::load(), I found that the function does too many things and has many error cases that need to be checked:

• It should throw if the cmd line args can not be parsed.
• It should throw if the file does not exist.
• It should throw if the file can not be parsed as ini file.
• It should throw if the window_width and window_height values are missing in the ini file.

Additionally, I need to check that create_window() is called with the correct arguments. This last test was easy with the gmock framework. However, if this test fails, the test does not show what part went wrong: Did parsing the cmd line args go wrong? Did parsing the ini file go wrong? Did I swap width and height?

In order to split the responsibilities and simplify testing, I decided to split the class:

• CmdLineToFileProject: Reads a file name from the cmd line args and calls load(file_name) on some IFileProject that was passed in the constructor.
• FileToStreamProject: Opens the file as stream and calls load(stream) on some IStreamProject.
• StreamToIniProject: Creates a IniConfig from the stream and calls load(ini) on some IIniProject.
• IniToFooConfigProject: Creates a FooConfig from the ini and calls load(config) on some IFooProject.

Now every class does exactly one thing. It is very easy to test both success and failure cases for each step. The FooProject is now very simple:

void FooProject::load(FooConfig const& f_config)
{
    m_window_system.create_window(f_config.m_width, f_config.m_height);
}

Unfortunately, the application initialization now became a lot more complicated. Previously, it was as simple as this:

FooProject project;
Application app(project);
app.run(argc, argv);

Now however, I need to chain all the project classes together:

FooProject p0;
IniToFooConfigProject p1(p0);
StreamToIniProject p2(p1);
FileToStreamProject p3(p2);
CmdLineToFileProject p4(p3);
Application app(p4);
app.run(argc, argv);

I think this is really inconvenient and the users of FooProject and Application are suffering. I just wanted to improve a small method (5 lines) and needed to create 4 new classes and their interfaces. How can you avoid this class chain? This must be a common problem when applying DI techniques. Are there any solutions to this? Did I get the whole application setup wrong?

Answer 1

Your code is confusing.

It seems to me that you can improve by doing two things

1. Use a DI container framework to resolve dependencies
2. Stop using constructors to pass in data

This would allow you to setup the code as follows

c = new container();
c.RegisterType<IFileReader, FileReader>();
c.RegisterType<ICmdLineReader, CmdLineReader>();
c.RegisterType<IFooConfigParser, FooConfigParser>();
... etc
c.RegisterType<FooProject>();

project = c.Resolve<FooProject>()
project.load(argc, argv)

FooProject's constructor takes in all the dependent types, but those types use a method to convert their input rather than taking data as a constructor. This means FooProject can call the methods and pass in the data itself rather than having to do it in the application layer while still maintaining the injectablity of the types

Answer 2

1. If something requires only a method, there is no need to make it a class just to follow the "do one thing" metapher. "Do one thing" does not mean one should convert every function into a class. If something requires not more than one function, especially a small static function, just keep it as a function.

   In case one really needs to inject a function into a class for mocking purposes, one can use std::function (C++ 11) for a constructor parameter to inject a functional object directly, without an extra polymorphic class.

2. read_file_name_from_cmd_line looks like a standalone static method for which it is probably acceptable to make it public for testing purposes. If you don't like having public methods in FooProject which are not part of the required interface, you can probably move read_file_name_from_cmd_line into a different class, maybe a general helper class for command line parsing.

3. A constructor call like std::ifstream file(file_name); does not require a test, this is part of the standard lib and should be tested by their authors. However, it might be a good idea to create a "seam" here (like a load method which takes directly an istream) allowing to replace the "real" file stream creation by a "memory stream creation" for testing purposes.

4. parse_ini_config could be a public member of IniConfig, which would make it simpler to test without any extra helper classes. A component IniConfig can usually be tested without mocking or DI.

5. So given all the steps in FooProject::load have unit tests, it is debatable whether FooProject::load needs a unit test on its own, or if an integration test (using a real file) would be enough. But even for a unit test, I would suggest to only mock out the "outer world I/O", which means the file access and m_window_system.create_window. There is no need to create a mock for every intermediate step on its own.

6. If the latter unit test fails for some input, and one cannot see the root cause immediately, then you one debug the test manually once, and identify the step or component which failed. Then should be to add a new unit test to the list of unit tests for that specific component which validates exactly that failure. There is usually no need to have the unit or integration test for FooProject::load showing the specific failing component, that is the responsibility of the tests for the individual components.