2
\$\begingroup\$

I'm running into a problem with my code in a ASP.NET MVC application. Maybe it's not a problem and I'm just being paranoid, but it seems like a problem to me (might be because I've just read the book Clean Code).

I am using the repository-pattern with the unit of work-pattern to abstract the data access. In between my controllers and the repositories I have the service-layer which handles my business logic.

Now my problem is: whenever I have a bigger controller with more functionality it gets cluttered up with service declarations. Here is a little example of what one of my controllers might look like:

public class ForumController
{
    private readonly ForumAnswerService _answerService;
    private readonly ForumThreadService _threadService;
    private readonly ReputationService _repService;
    private readonly IUnitOfWork _unitOfWork;

    public ForumController()
    {
        _unitOfWork = new EFContextUnitOfWork(new MyContext());
        _threadService = new ForumThreadService(new Repository<ForumThread>(_unitOfWork));
        _answerService = new ForumAnswerService(new Repository<ForumAnswer>(_unitOfWork));
        _repService = new ReputationService(new ReputationRepository(_unitOfWork));
    }

    /* more stuff which uses the services above.. */

    protected override void Dispose(bool disposing)
    {
        if (_unitOfWork != null)
        {
            _unitOfWork.Dispose();
        }
        base.Dispose(disposing);
    }
}

This does not seem so bad right now but I had controllers with even more services and with growing functionality I think the list of services might grow even bigger and it does not seem clean to me.

Here are some ideas I have to tackle the problem:

  1. Only initialize services when I actually need them (i.e. in the methods)

    • that will reduce the number of class variables but I think it might clutter up the methods and make them big and ugly
  2. Combine multiple services

    • maybe I just have too many small services and need to combine them, for example combine ForumThreadService and ForumAnswerService to a single ForumService
    • I don't like that though because of SRP and stuff
    • that would just move the clutter to the services because I would have to pass multiple repositories to the service
  3. Is that maybe a case where a DI framework would come in handy? Never used one and I only did constructor injection so I don't know..

  4. Nope, that's it. Only got those 3 ideas..

I hope my problem is clear and you guys can point me into the right direction.

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Yes, #3 is the one - this is exactly what Dependency Injection/Inversion of Control containers are meant for - removing the assembly part of the services from the main code.

Using an external DI framework will also give you additional benefits, such as stubbing out services while unit testing, allow future extensions to services without changing your main code, etc.

A good start to which framework to use you can find here: How do the major C# DI/IoC frameworks compare?

\$\endgroup\$
4
  • \$\begingroup\$ I would also look at #2. It seems like a very good possibility that there is duplicated code in those services which should be combined anyways. \$\endgroup\$ Feb 21, 2014 at 23:03
  • \$\begingroup\$ @JeffVanzella, since we don't know what's in these services, we can't say anything about any duplication in them. Anyway, as the system grows the number of services will eventually grow as well, and you end up with the same problem \$\endgroup\$
    – Uri Agassi
    Feb 22, 2014 at 5:22
  • \$\begingroup\$ Thanks a lot. I looked into the DI frameworks yesterday and it looks like it's what I need. \$\endgroup\$
    – mboldt
    Feb 22, 2014 at 7:59
  • \$\begingroup\$ @UriAgassi I was just saying it was something to look at \$\endgroup\$ Feb 22, 2014 at 22:26
1
\$\begingroup\$

There is a fourth option, namely dealing with the fact your Controller has "occasional" dependencies.

I'd be willing to bet your actions look something like this:

public ActionResult AddAnswer()
{
    _answerService.DoStuff(...);

    return View();
}

public ActionResult MarkThreadAsRead()
{
    _threadService.DoStuff(...);
    return View();
}

This results in your controller being instantiated, all dependencies created and then only some used depending on the action.

Now you can just switch to using a DI framework (If you only do one thing do that) however that doesn't address what I consider the underlying problem, which is the proliferation of dependencies due to the controller not having a clear responsibility.

Only you can really give the solution since it's heavily tied into your domain problem. At a guess from the code you've shown, it looks like you need a ThreadController and possibly AnswerController too. Then do something more than the default mvc route eg:

routes.MapRoute(name: "Forum",
            url: "forum/{id}",
            defaults: new { controller = "Forum", action = "Index", id = UrlParameter.Optional });

routes.MapRoute(name: "Thread",
            url: "forum/{forumid}/thread/{id}",
            defaults: new { controller = "Thread", action = "Index", id = 0 });

If you do decide just to switch to a DI framework then pick one that can handle injection of lazy Dependencies for you. eg:

public class FooController
{
    public Foo(Lazy<SomeRepo> aRepo){ ... }
}

The reason for that is when you're action doesn't require a unit of work connection (For example), then one isn't created and it's dependency on something expensive like a DB connection is not also created.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.