4

I am working on ASP.NET MVC3 solution that uses dependency injection with autofac. Our controllers are being created by autofac and properly and all required objects are being properly passed in. Those objects usually include services, repositories, and mappers converting domain object to MVC (view) models. So the controller constructor looks somewhat like:

public abcController(
        ILogger logger,
        IabcRepository abcRepository,
        IabcService abcService,
        IMapper<AbcDomain, AbcViewModel> abcMapper,
        ...
        )

Unfortunately, in time, those constructor parameter lists tend to grow quite quickly. Some of our controllers expect now 60 or more parameters.

Did we create some anti-pattern here?

EDIT

I should have mentioned that we try to follow thin contoller pattern. Also most of those parameters tend to be mappers - around 66%. The control methods are usually very simple, and follow either this pattern:

  • Based on parameters call appropriate service or repository
  • Use mapper to convert result to appropriate view model
  • Pass view model to view

Or this pattern:

  • Receive model from post action
  • Use mapper to convert it to appropiate domain object
  • Invoke appropriate service or repository with domain object
Sebastian K
  • 6,235
  • 1
  • 43
  • 67
  • You can use `injection by property` instead of `injection by constructor`. – Hamlet Hakobyan Jan 24 '13 at 22:22
  • @HamletHakobyan - Hmmm... that may work, I will have to try to see how it would work with autofac. I can see benefit of this solution when it comes to unit testing. Why don't you make it an answer. – Sebastian K Jan 24 '13 at 22:29
  • Also Autofac supports action injection see here http://stackoverflow.com/questions/5411881/how-to-do-action-injection-for-mvc-3-using-autofac. So parameters only needed in one action could be done that way. Still 60 is a lot :) – GraemeMiller Jan 24 '13 at 23:24
  • 1
    As a general rule of thumb, constructors should have **no more than 5 dependencies**. 60 dependencies is 12x too many dependencies. How do you even test a class with 60 dependencies? You might benefit from grouping [business operations inside their own class](http://bit.ly/s7tGEH). – Steven Jan 25 '13 at 09:36
  • Stevens code is what I based a huge amount of my architecture on. It really cleaned up my code that + some mapping stuff from Jimmy Bogard, various custom ActionResults and some custom ModelEnricher work. – GraemeMiller Jan 25 '13 at 12:29

4 Answers4

8

I can't really speak to how you should rearchitect your controller, though I agree with most of the other answers - 60 incoming parameters is a lot.

Something that might help reduce the number of parameters but not the number of dependencies is the Aggregate Service support that Autofac has.

Instead of taking 60 parameters directly, you can take a single aggregate parameter that has 60 properties on it.

You create an interface (just the interface, you don't actually have to implement it) with the dependencies:

public interface IMyAggregateService
{
  IFirstService FirstService { get; }
  ISecondService SecondService { get; }
  IThirdService ThirdService { get; }
  IFourthService FourthService { get; }
}

Then modify your controller to take that aggregate interface:

public class SomeController
{
  private readonly IMyAggregateService _aggregateService;

  public SomeController(
    IMyAggregateService aggregateService)
  {
    _aggregateService = aggregateService;
  }
}

You can register your aggregate service interface, your dependencies, and your controller and when you resolve the controller, the aggregate service interface will automatically be implemented and resolved for you.

var builder = new ContainerBuilder();
builder.RegisterAggregateService<IMyAggregateService>();
builder.Register(/*...*/).As<IFirstService>();
builder.Register(/*...*/).As<ISecondService>();
builder.Register(/*...*/).As<IThirdService>();
builder.Register(/*...*/).As<IFourthService>();
builder.RegisterType<SomeController>();
var container = builder.Build();

Again, it doesn't speak to the larger issue of requiring that many dependencies, but if you are just looking to simplify your constructor and the number of properties on the controller so it's more manageable, this is one strategy Autofac offers to help in that.

Check out the wiki page for more details.

Travis Illig
  • 23,195
  • 2
  • 62
  • 85
6

60 or more parameters is a lot.

In your question you stated "..Those objects usually include services, repositories, and mappers converting domain object to MVC (view) models..."

You've got a Fat Controller (not the Thomas The Task Engine kind) but a controller that is doing too much.

The balance I look for is Fat Model skinny controller. Ian Cooper talks about it well in this blog post

You can also look at things such as which parameters are actually cross cuttings concerns.

For instance Mapping and Logging in my mind are cross cutting concerns so you could potentially use Action Filters to clean up your controllers.

heads5150
  • 7,263
  • 3
  • 26
  • 34
  • Yeah, we actually trying to follow thin controller principles, I guess the problem arises from fact that we have separate mapper per each model class, and instead of few repositories we have many few-method repositories. But using mappers as cross cutting concern seems promising (logging is not a prolem). How would you go about using Action Filters for mapping purposes? – Sebastian K Jan 24 '13 at 22:33
  • Also see this question http://stackoverflow.com/questions/2420193/dependency-injection-constructor-madness and answer by Mark Seemann and Jimmy Bogard mvc conf2 presentation about thin controllers http://www.youtube.com/watch?v=8TYJjRxXnXs – GraemeMiller Jan 24 '13 at 22:37
  • 1
    Personally, I move most business logic out of the controllers to keep them slim. I typically use a simple messaging architecture where the controller sends commands into a messaging bus and a command handler executes the command. This way, each use case is nicely encapsulated, can be tested on its own and the handler has only the necessary dependencies to do its job. You don't have to use a fully blown asynchronous service bus framework for this, just a simple registry message type => handler type. You can use the DI container to resolve the handlers in order to inject dependencies. – Andre Loker Jan 24 '13 at 22:37
  • 1
    @GraemeMiller I will check those links - Mark Seemann wrote "Dependency Injection in .NET", so he should know the topic ;) – Sebastian K Jan 24 '13 at 22:49
  • Indeed he did :). I'd look at wrapping your mapping in a viewresult as I said in my answer . That would simplify things. – GraemeMiller Jan 24 '13 at 22:51
3

(Disclaimer: This answer was in regards to the size of the argument list. It does not reduce the dependencies within the controller)

In this case, you would inject a Factory.

For example:

interface IABCFactory {
    ILogger CreateLogger();
    IABCRepository CreateRepo();
    // .. etc
}

Your constructor then becomes:

private ILogger _logger;

public abcController(IABCFactory factory) {
    _logger = factory.CreateLogger();
    // .. etc
}

Note, you can inject into public properties.. but it is up to you whether you want to expose that to the outside world. If you don't want to break encapsulation, then you would go with a factory.

Simon Whitehead
  • 63,300
  • 9
  • 114
  • 138
  • 1
    Haven't you just moved the long list of params out of the controller and into the factory? Its not reduced, it has only been moved. – JK. Jan 24 '13 at 22:24
  • So your actual question isn't in regards to the size of your controllers' constructor argument list.. but to the dependencies your controller has? – Simon Whitehead Jan 24 '13 at 22:28
  • Also, it has been reduced. Your factory is reducing the amount of repeated code. Your constructor will have one argument per controller and 60 properties in one factory - instead of 60 arguments per controller. – Simon Whitehead Jan 24 '13 at 22:30
  • Its not my question :) But I do have the same concerns re: too many dependencies in the controller constructors – JK. Jan 24 '13 at 22:40
  • Sorry, I understood this "Unfortunately, in time, those constructor parameter lists tend to grow quite quickly. Some of our controllers expect now 60 or more parameters" to show concern about the number of arguments. I apologise for not answering your question. – Simon Whitehead Jan 24 '13 at 22:41
  • -1 The factory patttern itself isn't useful since he is using an ioc container to control the lifetimes. And using a service locator is a bad fit in this case since it hides the dependencies. – jgauffin Jan 25 '13 at 08:04
  • @jgauffin Most DI containers have extensions that remove the need to implement a service locator. I'm not sure an implementation detail that I omitted warrants a downvote :( – Simon Whitehead Jan 25 '13 at 08:26
  • You hide the dependencies. The class has still the same dependencies and nothing have been sovled. If you instead had focused on a specific problem like using a mapper factory (or facade) to remove all mappings from the constructor you'd get a +1. – jgauffin Jan 25 '13 at 08:30
3

If a lot of this is down to creating viewmodels this question and answer may help.

MVC - Controller with multiple select lists

Also I'd look at MVC 4 in Action from Manning. It covers creating an ActionResult that automates the mapping.

In my applications most of my controller actions are one line. Either pulling an entity and passing it to an Automapping and enriching viewresult or taking in a command and passing it to an action result that processes it

This blog post from Jimmy covers some of the POST side http://lostechies.com/jimmybogard/2011/06/22/cleaning-up-posts-in-asp-net-mvc/

Basically I get a domain object (from repo or other method) and return it with an automapped view result which maps to appropriate VM.

return AutoMappedView<ExaminationCreateModel>(new Examination ( _assetRepository.Find(assetId)));

The mapper ViewResult then passes it to an enricher (if one is found implementing IModelEnricher. See other stack question.

On return it is posted back as a command, command is then handled a bit like Bogard post.

    public virtual ActionResult Create(AddAssetExaminationCommand addAssetExaminationCommand, ICommandHandler<AddAssetExaminationCommand> addExaminationHandler) 
    {
        return ProcessForm(
            addAssetExaminationCommand,
            addExaminationHandler,
            RedirectToAction(MVC.OnboardAsset.Examinations.Create()),
            RedirectToAction(MVC.OnboardAsset.Examinations.Index(addAssetExaminationCommand.AssetId)));
    }

If it fails validations then it redirects to GET and the Modelstate is merged (PRG pattern using something like this) so errors persist. If it is valid command handler deals with it and we redirect to a success page

Community
  • 1
  • 1
GraemeMiller
  • 11,973
  • 8
  • 57
  • 111