10

I've been trying to implement a loosely coupled application in an asp.net MVC5 app. I have a controller:

public class HeaderController : Controller
    {
        private IMenuService _menuService;

        public HeaderController(IMenuService menuService)
        {
            this._menuService = menuService;
        }

        //
        // GET: /Header/
        public ActionResult Index()
        {

            return View();
        }


        public ActionResult GetMenu()
        {

            MenuItem menu = this._menuService.GetMenu();
            return View("Menu", menu);

        }

    }

And service being used in this controller is:

public class MenuService : IMenuService
{
    private IMenuRespository _menuRepository;

    public MenuService(IMenuRespository menuRepository)
    {
        this._menuRepository = menuRepository;
    }

    public MenuItem GetMenu()
    {
        return this._menuRepository.GetMenu();
    }
}

And the repository being used in the service class is:

public class MenuRepository : IMenuRespository
    {
        public MenuItem GetMenu()
        {
            //return the menu items
        }
    }

The interfaces used for the service and repository are as such:

 public interface IMenuService
    {
        MenuItem GetMenu();
    }

public interface IMenuRespository
    {
        MenuItem GetMenu();
    }

The constructor for HeaderController takes in the MenuService using Constructor Injection, and I have ninject as the DI container handling this.

It all works great - except, in my controller, I can still do this:

MenuItem menu = new MenuService(new MenuRepository());

...which breaks the architecture. How can I prevent the 'new' being used in this way?

M.R.
  • 4,737
  • 3
  • 37
  • 81
  • 2
    Just don't do that? Not sure why this is more problematic than naming controller `HeaderCantroller` "which breaks the architecture"... Is there particular reason you are looking for it (like particular pattern that can't be stopped with good naming/code review)? – Alexei Levenkov Apr 21 '15 at 02:54
  • Can you not validate the argument inside the constructor and throw an exception if its being used incorrectly? – Ron Beyer Apr 21 '15 at 02:55
  • 1
    @AlexeiLevenkov: Using the constructor directly violates 'separation of concerns' and tightly couples HeaderController to MenuService and MenuRepository. – M.R. Apr 21 '15 at 03:07
  • @RonBeyer: How would I do that? I mean, if I instantiate the classes 'new', its still correct, as the constructors are getting the correct object type. I want to prevent programmers to call it directly, because it would tightly couple them. – M.R. Apr 21 '15 at 03:09
  • Ahh, well then I would use a factory pattern and make the constructor internal. Or you can make the constructor private and make static methods to create new instances the right way. You show the wrong way to use the MenuService/Repository, what is the right way? – Ron Beyer Apr 21 '15 at 03:11
  • @RonBeyer - actually, I have ninject as the DI container which injects MenuService into HeaderController at instantiation time. I believe most DI containers would do this as well... I think I will look into the factory pattern. Thanks! – M.R. Apr 21 '15 at 03:15
  • M.R. - you can combine @RonBeyer's suggestion with BrokenGlass answer to make very hidden (short of reflection) constructors (see also my comment to that answer) – Alexei Levenkov Apr 21 '15 at 03:20
  • Short of telling other developers not to use the services in this manner, or hiding references in other patterns etc. You could introduce a custom code analysis rule set to detect this usage on build, and throw errors accordingly.... – Brent Mannering Apr 21 '15 at 03:33
  • @M.R.: Did you find the proper design model of the project? – Jitendra Pancholi Apr 21 '15 at 05:54
  • 2
    Related: https://stackoverflow.com/questions/9501604/ioc-di-why-do-i-have-to-reference-all-layers-assemblies-in-entry-application – Steven Apr 21 '15 at 06:46

4 Answers4

8

One way to do it would be to move your interfaces and implementations into separate Visual Studio projects / assemblies and only reference the implementation project in the project(s) that actually needs it - everything else can reference the interface project for your IMenuService - at that point the code can consume the interface, but not actually new up any implementations itself.

You can then reference the implementation project wherever you DI in your dependencies.

WebApp Solution:

WebApp Proj (Controllers etc.) --> Service Interface Proj

Service Impl Project --> Service Interface Proj

Even so this is a good approach, it's not fool proof by all means - the other component is education and code review to come up with best practices that work for your team such as testability and dependency injection.

Community
  • 1
  • 1
BrokenGlass
  • 158,293
  • 28
  • 286
  • 335
  • Hmm - but doesn't this code: MenuItem menu = this._menuService.GetMenu(); need a reference to the actual implementation of MenuService? – M.R. Apr 21 '15 at 03:11
  • sure - but you don't know anything about the instance that is being used - other than that it implements `IMenuService` – BrokenGlass Apr 21 '15 at 03:13
  • 1
    But if I have the reference, I can still just call new MenuService(new MenuRepository()), no? – M.R. Apr 21 '15 at 03:14
  • No you cannot - because the project where `MenuService` is defined is not referenced by your webapp project - only the project where `IMenuService` is defined is – BrokenGlass Apr 21 '15 at 03:15
  • 1
    Right - but I guess regarding my original question - if I am calling this._menuService.getMenu(), which needs the actual implementation, I HAVE to add a reference to the project that has the implementation for that method, no? Can I JUST reference the interface project, and it will know to find the implementation, even without a reference? – M.R. Apr 21 '15 at 03:17
  • 1
    Good suggestion. Additionally you can make all implementations internal and provide "RegisterAllInternalClassesWithContainer" method to prevent any reasonable chance of creating objects directly... Will tie your implementation with container (which can again be solved "by another level of indirection" via friend assembly to register classes). – Alexei Levenkov Apr 21 '15 at 03:18
  • @M.R.that's exactly why you should return interface types and not implementation types if possible – BrokenGlass Apr 21 '15 at 03:20
  • @AlexeiLevenkov: How can be the implementation of a method be internal? it must be public. – Jitendra Pancholi Apr 21 '15 at 05:50
  • 1
    @JitendraPancholi what must be public? Interface implementation can be explicit (somewhat more than private) if you really want to and classes themselves can be internal, maybe you'll need public constructors if it required by your DI container... – Alexei Levenkov Apr 21 '15 at 05:53
  • @AlexeiLevenkov: Yes, you're correct, i forgot about explicit implementation. I just implement the methods explicitly and now Controller can't access them even by directly instantiating the implemented class. You resolved the concern. :) – Jitendra Pancholi Apr 21 '15 at 06:16
4

I assume part of the issues with manually instantiating the object may come with working with a large team, whereby some members are using the constructor injection technique the wrong way. If that is the case, I found pretty much by educating them on the framework resolved most of the issues. Occasionally, you would find someone doing it the wrong way, but not often. Another alternative could be to add an [EditorBrowsable(EditorBrowsableState.Never)] attribute on the controller constructor. The constructor will disappear from intellisense; well, it will appear to be gone. It can still be used, however.

You could break out the implementations into another DLL not directly references (implicitly referenced) by the MVC project, and thus since there isn't a direct reference, you can't use those types directly. With the interfaces in one project, which each project references, and the project with the implementations indirectly referenced, only the interfaces would thus be included. I'd recommend including a direct reference in the unit test project, if you are doing unit tests, to enhance test coverage.

Brian Mains
  • 50,520
  • 35
  • 148
  • 257
  • Tell people not to use it :-) ? What is this answer? IF someone doesn't follow the guidelines, how would you restrict them programmatically? And the question is Why should we implement Interfaces if people can access the actual implementation directly? – Jitendra Pancholi Apr 21 '15 at 05:42
  • 3
    @JitendraPancholi: This is actually a very good answer. This is a general problem I see with architects, who rather try to prevent people from being able to do things wrong, which often lead to very complex useless code structures where the developers still find a massive amount of things to do wrong and ways to work around the architect's restrictions. Instead, developers should be educated and the team should have code reviews, because those are much more effective. – Steven Apr 21 '15 at 06:50
  • @Steven: Instead of such restrictions by guidelines, use the approach mentioned by BrokenGlass & AlexeiLevenkov in above answer to create a good architecture to reduce human errors. – Jitendra Pancholi Apr 21 '15 at 07:24
  • @JitendraPancholi: Did you read [this](https://stackoverflow.com/questions/9501604/ioc-di-why-do-i-have-to-reference-all-layers-assemblies-in-entry-application)? – Steven Apr 21 '15 at 08:02
  • @Steven: Yes I read. But If I do explicit implementation of interface methods, what's wrong in this? This would also restrict the programmer to directly instantiate the repository class and use their methods. They must have to use interface and also I'm taking references of Business Layer in main application, which is explained in link given by you. What's wrong here? – Jitendra Pancholi Apr 21 '15 at 09:00
  • @Steven: As explained in the link, if we take the reference of our DAL in our main application, Would it good as per DI? – Jitendra Pancholi Apr 21 '15 at 09:03
  • @JitendraPancholi: Yes. As the answer describes, it's about layering. Your main application assembly (in MVC) contains two layers: the composition root layer and the UI layer and there is nothing wrong with this, because layers are about logical structuring, not physical structuring. The composition root layer depends on the DAL classes, while the UI layer should only depend on the DAL interfaces. The DAL interfaces can be in the same assembly as the classes, but still in a different layer. – Steven Apr 21 '15 at 09:25
  • @JitendraPancholi I assume part of the problem is it is a large team and oftentimes some developers will do exactly as he mentioned. I had that problem myself, where people will instantiate the object directly and spent effort doing it the way they weren't supposed to. I also recommended including the implementations in a different project; however, you have to apply care in how you do it... – Brian Mains Apr 21 '15 at 10:26
  • 1
    @Steven, BrianMains: Thanks for the healthy discussion. I enjoyed it a lot. :D :) – Jitendra Pancholi Apr 21 '15 at 10:33
3

Couple of potential options (which I've never tried, but might have some legs):

  • you could maybe write an FXCop rule which errors if the constructor is used in the code.

  • you could mark the constructor as obsolete, and have the build server fail if you use obsolete methods in the code.

If the DI container uses it through reflection this should all be ok (although in the FXCop case you could probably not throw if it was in a method in the NInject namespace)

Sam Holder
  • 32,535
  • 13
  • 101
  • 181
  • Better to explicitly implements interface methods, so If anyone tries to instantiate the class and access the methods, so it won't even appear due to access level. – Jitendra Pancholi Apr 21 '15 at 09:50
  • 1
    @JitendraPancholi ok, but those options are covered in other answers, I was trying to give some alternatives which might be possible. I didn't realise there was only one way to skin this cat. – Sam Holder Apr 21 '15 at 09:52
2

As general design principle, interfaces (Contracts) should be in one assembly and the implementation should in another assembly. The Contracts assembly should be reference in MVC project and implemented assembly should be copied in "bin" folder. Than use "Dynamic Module Loading" to load types. In this way you will avoid the above mentioned problem and this is more extensive solution. Because you can replace implementation without building UI and Contact Assemblies.

Murshad.NET
  • 29
  • 1