Is having many build-once factories a sign of bad dependency injection design?

Question

I have a form. It contains things like grids. Users do things with these grids and what they do with them is sent to a SQL server. Clearly, the server is a volatile dependency and should be dependency injected.

Of course, it would be silly to have the form know about my business logic, so all of the business logic to do with the grids is in individual classes. Let's call where these classes live my "business logic layer" and where my form the lives the "UI layer". Since my classes in the business logic layer need to know about the server and the server is a volatile dependency, those classes should take the relevant details about the server in their constructor. For example, the class holding the business logic for FooCells in the grid (let's call it FooCellsHandler) should take an IFooSqlRepository in its constructor and that IFooSqlRepository parameter should be passed a FooSqlRepository.

So far, our composition root looks something like this:

private static RootType CompRoot(string SqlConnString)
{
    return
        new Form1(
            new FooCellsHandler(
                new FooSqlRepository(
                    new SqlContext(SqlConnString)
                )
            )
        )
    ;
} 

And although the indentation is ugly, it's all fine... As long as the only argument that FooCellHandler needs is the IFooSqlRepository. If it needs extra arguments at runtime, let's say CellHeader and CellValue, then we can't have those in our composition root. We therefore need to make a factory. Our composition root will still be the same, but with FooCellsHandler replaced with FooCellsHandlerFactory. This factory object will only constructed once in the entire application. Here is one such possible FooCellsHandler and its factory.

public class TimeCellHandler
{
  private readonly int _value;
  private readonly string _header;
  private readonly ITimeCellSqlRepository _repo;
  
  public TimeCellHandler(int CellValue, string CellHeader, ITimeCellSqlRepository Repo)
  {
      _value = CellValue;
      _header = CellHeader;
      _repo = Repo;
  }

  private void FormatForServer()
  {
    // Helper method used by others.
  }
  
  public void DeleteFromServer()
  {
     // Deletes values corresponding to this time cell from the server.
  }

  public void InsertToServer()
  {
     // Inserts values corresponding to this time cell to the server.
  }
}

public class TimeCellHandlerFactory
{
    private readonly ITimeCellSqlRepository _repo;
    public TimeCellHandlerFactory(ITimeCellSqlRepository Repo)
    {
        _repo = Repo;
    }

    public TimeCellHandler MakeHandler(int CellValue, string CellHeader)
    {
        return TimeCellHandler(CellValue, CellHeader, _repo);
    }
}

Is having many build-once factories a sign of bad dependency injection design? I see many reasons to be suspicious of it. Because my form handles many different things, I can imagine that my composition root will contain many such factories and begin to look something like the below. This seem ugly, repeats my SQL information a great many times, and puts many layers of indirection between what my form is supposed to do and how it is being done.

private static RootType CompRoot2(string SqlConnString)
{
    return
        new Form1(
            new FooCellsHandlerFactory(
                new FooSqlRepository(
                    new SqlContext(SqlConnString)
                )
            ),
            new BarCellsHandlerFactory(
                new BarSqlRepository(
                    new SqlContext(SqlConnString)
                )
            ),
            new OtherCellsHandlerFactory(
                new OtherSqlRepository(
                    new SqlContext(SqlConnString)
                )
            ),
            new MyColumnHandlerFactory(
                new MyColumnSqlRepository(
                    new SqlContext(SqlConnString)
                )
            ),
            new EdgeCaseHandlerFactory(
                new EdgeCaseSqlRepository(
                    new SqlContext(SqlConnString)
                )
            ),
        )
    ;
} 

Answer 1

Your doubts about the design are caused by the amount of boilerplate and repetitous looking code - so lets try to fix this. Then you can answer yourself afterwards whether the design is "good enough" in your perception.

1. First thing I would question here is if each of your factories really needs a new SqlContext object. Creating a new SqlContext once and pass it into all factories could remove some duplicate logic here and simplify the code a little bit. In case that's not possible, you may consider to avoid the repetition of the SqlConnString utilizing a Func like this:

   private static RootType CompRoot2(string SqlConnString)
   {
       var createSqlContext = () => new SqlContext(SqlConnString);
   
        new Form1(
              new FooCellsHandlerFactory(
                  new FooSqlRepository(createSqlContext())
              ),
              new TimeCellsHandlerFactory(
                  new TimeSqlRepository(createSqlContext())
              ),
              // ...
   

   Note createSqlContext is effectively a closure.

2. Now let me address the large number of factories. Your form contains several different cell types which need to be handled differently.You may consider to use only one kind of object called CellHandler or CellHandlerService, which is passed into the form. This object can internally have different "special handlers" (FooCellHandler, BarCellsHandler, ... ) for the individual cell types (maybe using different classes, maybe not), and also the factories for those cell types. This approach is known as "introducing a Facade Service", as Mark Seemann called it. It will not necessarily reduce the overall number of factories in your program, but reduce the number of constructor parameters of your form.

   I would also expect to have some common logic in the new service class which can be shared for all types of cells, maybe even a shared repo for all of them, or a shared SqlContext (but this is impossible to tell without actually seeing the real code). Maybe it makes it possible to have a more generic kind of factory which can produce different types of cells in a parametric fashion. Another approach could be to make use of the Prototype design pattern inside the service, which might also reduce the number of required factories. But even if you cannot avoid the different handler types internally, or cannot simplify the logic, the responsibilities in your code will become clearer this way.

3. When your factory classes are effectively just a container for one function with one bound variable, consider to replace them by factory functions in form of closures. This isn't a change in the overall design, but reduces the amount of boilerplate code, since you do not have to write a full-blown factory class for each and every case. For example:

   private static RootType CompRoot2(string SqlConnString)
   {
       var createSqlContext = () => new SqlContext(SqlConnString);
   
       var fooRepo = new FooSqlRepo(createSqlContext());
       var fooCellsHandlerFactory= () => new FooCellsHander(); 
   
       var timeRepo = new TimeSqlRepo(createSqlContext());
       var timeCellHandlerFactory =  (CellValue, CellHeader) 
                       => new TimeCellHandler(CellValue, CellHeader, timeRepo);
   
        new Form1(
              fooCellsHandlerFactory,
              timeCellHandlerFactory, 
              // ...
   

   This example should also answer your former question how to avoid "escort classes".

Answer 2

A composition root doesn't have to stand completely alone. Sure, it gives you a lot of flexibility if it's flat and procedural but the longer that gets the more it numbs the brain. Always keep in mind what it's going to be like maintaining this code.

It is perfectly ok to break up this code and make something else deal with some of this complexity. Keep what's managed here simple and accessible. That way, looking at it, and changing it, is relaxing. Not overwhelming.

A humble example:

private static Form1 Form1Factory(string SqlConnString)
{
    return
        new Form1(
            new FooCellsFactoryBuilder(
                new SqlContext(SqlConnString)
            ).build(),
            new BarCellsFactoryBuilder(
                new SqlContext(SqlConnString)
            ).build(),
            new OtherCellsFactoryBuilder(
                new SqlContext(SqlConnString)
            ).build(),
            new MyColumnFactoryBuilder(
                new SqlContext(SqlConnString)
            ).build(),
            new EdgeCaseFactoryBuilder(
                new SqlContext(SqlConnString)
            ).build(),
        )
    ;
} 

Answer 3

Its really the interaction between the model and the repository which I would focus on.

In your example you have a TimeCellHandler, which has the value, cell and repository encapsulated and various Save CRUD methods.

You don't show what methods you have on the repository or Form.

Lets imagine though that we have a Root object as such:

Grid
{
    List<Cell> Cells
}

Cell
{
    string CellType
    string ValueAsString(){..}
    int X,
    int Y
}

Cell<T> : Cell
{
   T value 
}

Now I can have a repository

GridRepository
{
    void SaveGrid(Grid grid); //saves whole grid
    //if needed
    void SaveCell(string gridId, Cell cell);
    ...etc
}

And Form

Form
{
    Grid MyGrid
    IGridRepository repo

    void Save()
    {
         //call grid repo methods to save grid, or portions of grid as required.
    }
}

Instead of having the save methods on the model, they go on a repository which takes the model as a parameter. This saves you injecting the repo and related issues.

Now you might complain that You will have to cast the Cell to the correct type via reflection to get at the underlying typed value. There are some workarounds you could use to avoid this, but I would argue that its fine in what basically amounts to a serialisation layer.