How to reduce number of injected dependencies on controller

Question

I am using MVC3, Entity Framework v4.3 Code First, and SimpleInjector. I have several simple classes that look like this:

public class SomeThing
{
    public int Id { get; set; }
    public string Name { get; set; }
}

I have another entity that looks like this:

public class MainClass
{
    public int Id { get; set; }
    public string Name { get; set; }
    public virtual AThing AThingy { get; set; }
    public virtual BThing BThingy { get; set; }
    public virtual CThing CThingy { get; set; }
    public virtual DThing DThingy { get; set; }
    public virtual EThing EThingy { get; set; }
}

Each Thingy (currently) has its own Manager class, like so:

public class SomeThingManager
{
    private readonly IMyRepository<SomeThing> MyRepository;

    public SomeThingManager(IMyRepository<SomeThing> myRepository)
    {
        MyRepository = myRepository;
    }
} 

My MainController consequently follows:

public class MainController
{
    private readonly IMainManager MainManager;
    private readonly IAThingManager AThingManager;
    private readonly IBThingManager BThingManager;
    private readonly ICThingManager CThingManager;
    private readonly IDThingManager DThingManager;
    private readonly IEThingManager EThingManager;

    public MainController(IMainManager mainManager, IAThingManager aThingManager, IBThingManager bThingManager, ICThingManager cThingManager, IDThingManager dThingManager, IEThingManager eThingManager)
    {
        MainManager = mainManager;
        AThingManager = aThingManager;
        BThingManager = bThingManager;
        CThingManager = cThingManager;
        DThingManager = dThingManager;
        EThingManager = eThingManager;
    }

    ...various ActionMethods...
}

In reality, there are twice as many injected dependencies in this controller. It smells. The smell is worse when you also know that there is an OtherController with all or most of the same dependencies. I want to refactor it.

I already know enough about DI to know that property injection and service locator are not good ideas.

I can not split my MainController, because it is a single screen that requires all these things be displayed and editable with the click of a single Save button. In other words, a single post action method saves everything (though I'm open to changing that if it makes sense, as long as it's still a single Save button). This screen is built with Knockoutjs and saves with Ajax posts if that makes a difference.

I humored the use of an Ambient Context, but I'm not positive it's the right way to go. I humored the use of injecting a Facade as well. I'm also wondering if I should implement a Command architecture at this point. (Don't all of the above just move the smell somewhere else?)

Lastly, and perhaps independent of the three above approaches, is should I instead have a single, say, LookupManager with explicit methods like GetAThings(), GetAThing(id), GetBThings(), GetBThing(id), and so on? (But then that LookupManager would need several repositories injected into it, or a new type of repository.)

My musings aside, my question is, to reiterate: what's a good way to refactor this code to reduce the crazy number of injected dependencies?

Answer 1

Using a command architecture is a good idea, since this moves all business logic out of the controller, and allows you to add cross-cutting concerns without changes to the code. However, this will not fix your problem of constructor over-injection. The standard solution is to move related dependencies into a aggregate service. However, I do agree with Mark that you should take a look at the unit of work pattern.

Answer 2

Have you considered using a unit of work design pattern? There is a great MSDN post on what a unit of work is. An excerpt from that article:

In a way, you can think of the Unit of Work as a place to dump all transaction-handling code. The responsibilities of the Unit of Work are to:

• Manage transactions.
• Order the database inserts, deletes, and updates.
• Prevent duplicate updates. Inside a single usage of a Unit of Work object, different parts of the code may mark the same Invoice
  object as changed, but the Unit of Work class will only issue a
  single UPDATE command to the database.

The value of using a Unit of Work pattern is to free the rest of your code from these concerns so that you can otherwise concentrate on business logic.

There are several blog posts about this, but the best one I've found is on how to implement it is here. There are some other ones which have been referred to from this site here, and here.

Lastly, and perhaps independent of the three above approaches, is should I instead have a single, say, LookupManager with explicit methods like GetAThings(), GetAThing(id), GetBThings(), GetBThing(id), and so on? (But then that LookupManager would need several repositories injected into it, or a new type of repository.)

The unit of work would be able to handle all of these, especially if you're able to implement a generic repository for most of your database handling needs. Your tag mentions you're using Entity Framework 4.3 right?

Hope this helps!

Answer 3

I think your main issue is too many layers of abstraction. You are using Entity Framework, so you already have a layer of abstraction around you data, adding two more layers (one per entity) via a Repository and a Manager interface has led to the large number of interfaces your controller depends upon. It doesn't add a whole lot of value, and besides, YAGNI.

I would refactor, getting rid of your repository and manager layers, and use an 'ambient context'.

Then, look at the kinds of queries your controller is asking of the manager layers. Where these are very simple, I see no problems querying your 'ambient context' directly in your controller - this is what I would do. Where they are more complicated, refactor this into a new interface, grouping things logically (not necessarily one per Entity) and use your IOC for this.