I've just come from a place where an API controller would have just the Services it needed injected in to it...

[ApiController]
public class SomeController : ControllerBase
{
    private IFirstService firstService
    private ISecondService secondService
    public SomeController(IFirstService firstService, ISecondService secondService)
    {
        this.firstService = firstService;
        this.secondService = secondService;
    }

    [HttpGet]
    public IActionResult SomeMethod()
    {
        var data = firstService.GetSomething();
        return OkObjectResult(data);
    }
}

Now I find myself in a shop that does this...

[ApiController]
public class SomeController : ControllerBase
{
    private IServiceProvider services;
    public SomeController(IServiceProvider services)
    {
        this.services = services;
    }

    [HttpGet]
    public IActionResult SomeMethod()
    {
        var service = servies.Get<IFirstService>();
        if(service is null)
        {
            //...
        }
        var data = firstService.GetSomething();
        return OkObjectResult(data);
    }
}

Now, I can't really explain why, but this just seems wrong.

Am I just experiencing StuckInMyWaysitis or is this really the bad practice my bones tells me it is? Or, is there, in fact, a more widely accepted way of doing the "right" thing?

CodePudding user response：

It's wrong because it means that every time you need a service you have to explicitly request it and then check the instance for null. This is unnecessary code duplication for no benefit.

It also violates the explicit dependencies principle, which Microsoft recommends you use to architect your code.

Almost certainly this was done because somebody couldn't figure out how DI works, or they forgot to register a service and couldn't be a***d to fix it properly, so they just chucked in IServiceProvider instead and that ended up working, and then they used it everywhere. In other words, laziness and/or stupidity.

Likely you will come up against resistance when you try to fix this by using explicit dependencies. The trick is to make the person(s) advocating for this mess explain why the mess is better than following good architectural practices, particularly those from Microsoft.

When you've been programming long enough, you learn to trust your gut. If it feels bad, it almost always is.

CodePudding user response：

Injecting IServiceProvider implements the Service Locator pattern, which is generally considered to be an anti-pattern.

In your first example two services are injected. You can easily tell what the controller depends on. It's easier to tell if a class begins to depend on too many things when we see five, 10, or 20 dependencies injected. When that happens we usually refactor because the number of dependencies indicates that the class is doing too many things.

In the second example we can't tell from the injected dependency (IServiceProvider) what the class depends on. The only way to tell is to look at every use of services throughout the class and see what gets resolved from it. A class could end up depending on many other classes even though we only see one dependency in the constructor.

This also makes unit testing more difficult. In the first example we might have to create fakes or mocks for one or both services. In the second example we have to either mock IServiceProvider to return mocks or create an IServiceCollection, register the mocks with it as service implementations, and then build a ServiceProvider from it. Both make tests more complex.

Some have reasoned that API controllers are an exception, and that it's okay to have them depend on something like a service locator. (MediatR is a common example.) This is an opinion: It's not bad as long as the controller has little or no logic and is only used to route HTTP requests to some higher-level code.

If we use MediatR or some similar abstraction like ICommandHandler<TCommand> then at least we've constrained the class to submitting queries or commands to handlers. It's not as bad as injecting IServiceProvider which allows the class to resolve any registered service.

CodePudding user response：

First, Let us refactor the second code to get rid of some code smells,

[ApiController]
public class SomeController : ControllerBase
{
    private IFirstService firstService
    private ISecondService secondService
    private IServiceProvider services;

    public SomeController(IServiceProvider services)
    {
        this.services = services;
        this.firstService= servies.Get<IFirstService>();
        this.secondService= servies.Get<ISecondService>();

    }

    [HttpGet]
    public IActionResult SomeMethod()
    {
        var data = firstService.GetSomething();
        return OkObjectResult(data);
    }
}

Why?

1. you automatically get rid of all the checks, and now you can do that in your constructor if needed.
2. If many methods needed instances all might have duplicate codes like this.
3. It violates SRP as the methods are doing more than they should be.

Now if we look it is closer to your First code. With one difference, Instantiating service vs Injecting service. There are a few problems with this IMO,

1. DI Containers are tools, they are not part of our domain. By taking IServiceProvider, we are trying our services to them. Which implies we always need some DI provider.

2. Secondly this also hides our dependencies, which makes integration difficult. Constructors are like messengers that clearly tell us what we need to keep ready beforehand, before we instantiate a Class. If we hide this information, you may not know if certain dependency was configured or not without running the application. With clearly defined dependencies in constructor, we cannot skip this part.

3. Also, just like we had duplicate code in our methods, now we have duplicate code in constructor of different services. Each service will be calling these Get methods. So why not do them in one place. And if you consider this and refactor, you automatically reach to your first example.

[ApiController]
public class SomeController : ControllerBase
{
    private IFirstService firstService
    private ISecondService secondService
    public SomeController(IFirstService firstService, ISecondService secondService)
    {
        this.firstService = firstService;
        this.secondService = secondService;
    }
}

public class Startup()
{
   public void Start()
   {
     //.... 
     //....
     var service1 = servies.Get<IFirstService>();
     var service2 = servies.Get<IFirstService>();
     SomeController= new Controller(service1,service2); 
     //or just servies.Get<SomeController>();
   } 
}