How to reduce number of injected dependencies on controller
Asked Answered
E

3

8

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?

Escrow answered 20/8, 2012 at 23:42 Comment(1)
possible duplicate of How to deal with constructor over-injection in .NETConcenter
C
5

Using a command-handler 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 Facade Service. However, I do agree with Mark that you should take a look at the unit of work pattern.

Concenter answered 21/8, 2012 at 7:32 Comment(4)
Thanks for the references. I like this approach. I'll refactor to an aggregate service for my immediate needs and then take a deeper look at command architecture and unit of work.Escrow
@Concenter can you provide an example of how this might work with SimpleInjector. I've been following your example at cuttingedge.it/blogs/steven/pivot/entry.php?id=95 and it's been a useful approach. How might I apply a facade or aggregate service.Gautier
@DavidClarke: I'm not sure what to show. Take a look at the Unit of Work pattern; that is typically a class that would wrap repositories. To prevent constructor over-injection in the unit of work, you can inject a factory that knows how to resolve repositories.Concenter
@Concenter so when using the command architecture, how do you avoid constructor over-injection? It seems that each action would need at least one handler and most of my controllers have more than 5 actions. The aggregate service described here doesn't seem to apply as surely you shouldn't be "[hiding] the aggregate behavior behind a new abstraction" for a command which already has a single responsibility? Did you mean to suggest Parameter Objects instead? (another concept mentioned in the same link).Fathom
M
4

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!

Melson answered 21/8, 2012 at 1:4 Comment(4)
Here is (yet) another variation of the unit of work pattern.Concenter
Do I create just a sungle UnitOfWork class that all my controllers use? I already have a GenericRepository<T>. That class would reference every repository?Escrow
Yep that's exactly what this would do. This single unit will also manage the concurrency of items when they're being pulled from all the different repos, as well as ensure you only have 1 shared context between them.Melson
I would then end up with a single UnitOfWork class with constructor over-injection. It looks like I need aggregate service(s) no matter what.Escrow
A
0

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.

Amara answered 21/8, 2012 at 1:11 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.