C# – Is This Testable Code Written Correctly?

asp.net-mvccdependency-injectionunit testing

I have an interface called IContext. For the purposes of this it doesn't really matter what's it does except the following:

T GetService<T>();

What this method does is look at the current DI container of the application and attempts to resolve the dependency. Fairly standard I think.

In my ASP.NET MVC application, my constructor looks like this.

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetService<ISomeService>();
    AnotherService = ctx.GetService<IAnotherService>();
}

So rather than adding multiple parameters in the constructor for each service (because this will get really annoying and time-consuming for the developers extending the application) I am using this method to get services.

Now, it feels wrong. However, the way I'm currently justifying it in my head is this – I can mock it.

I can. It wouldn't be difficult to mock up IContext to test the Controller. I'd have to anyway:

public class MyMockContext : IContext
{
    public T GetService<T>()
    {
        if (typeof(T) == typeof(ISomeService))
        {
            // return another mock, or concrete etc etc
        }

        // etc etc
    }
}

But as I said, it feels wrong. Any thoughts / abuse welcome.

Best Answer

Having one instead of many parameters in the constructor is not the problematic part of this design. As long as your IContext class is nothing but a service facade, specificially for providing the dependencies used in MyControllerBase, and not a general service locator used throughout your whole code, that part of your code is IMHO ok.

Your first example might be changed to 

protected MyControllerBase(IContext ctx)
{
    TheContext = ctx;
    SomeService = ctx.GetSomeService();
    AnotherService = ctx.GetAnotherService();
}

that would not be a substantial design change of MyControllerBase. If this design is good or bad depends only on the fact if your want to

  • make sure TheContext, SomeService and AnotherService are always all initialized with mock objects, or all of them with real objects
  • or, to allow initialization them with different combinations of the 3 objects (which means, for this case your would need to pass the parameters individually)

So using only one parameter instead of 3 in the constructor can be fully reasonable.

The thing which is problematic is IContext exposing the GetService method in public. IMHO you should avoid this, instead keep the "factory methods" explicit. So will it be ok to implement the GetSomeService and GetAnotherService methods from my example using a service locator? IMHO that depends. As long the IContext class keeps just beeing a simple abstract factory for the specific purpose of providing an explicit list of service objects, that is IMHO acceptable. Abstract factories are typically just "glue" code, which don't have to be unit-tested itself. Nethertheless you should ask yourself if in the context of methods like GetSomeService, if you really need a service locator, or if an explicit constructor call would not be simpler.

So beware, when you stick to a design where the IContext implementation is just a wrapper around a public, generic GetService method, allowing to resolve any arbitrary depencencies by arbitrary classes, then everything applies what @BenjaminHodgson wrote in his answer.

Related Topic