4

I have been wondering for quite some time on some issues while using dependency injection:

In a layered application, I normally inject repositories into the application service using constructor injection:

public class SomeService
{
    private IRepositoryA _repoA;
    private IRepositoryB _repoB;
    private IRepositoryC _repoC;

    public SomeService(IRepositoryA repositoryA, 
                       IRepositoryB repositoryB, 
                       IRepositoryC repositoryC
                       /* Other dependencies*/)
    {
        _repoA = repositoryA;
        _repoB = repositoryB;
        _repoC = repositoryC;
    }

    public void SomeMethod1()
    {
        //Do something with repo A and B
    }

    public void SomeMethod2()
    {
        //Do something with repo C
    }

    //Other methods
}

The problems with this are:

  • When the service becomes complicated, the number of dependencies will grow more and more, and soon the constructor will become ugly with lots of parameters. It might be the service is doing too many things (violate SRP) and better be separated into several classes, but I think this situation is not uncommon so I'm curious how others solve this

  • SomeMethod1() only uses repo A and B, but when client calls it, the service is created with all repositories, meaning it has more dependencies than it needs.

To address those 2 issues, I see some people have another abstraction, which is repository factory

public interface IRepositoryFactory
{
    T GetRepository<T>() where T : IRepository;
}

public class RepositoryFactory
{
    public T GetRepository<T>()
    {
        //Use IoC to return correct IRepository
    }
}

and inject factory into service class:

public class SomeService
{
    private IRepositoryFactory _factory;
    public SomeService(IRepositoryFactory factory)
    {
        _factory = factory;
    }

    public void SomeMethod1()
    {
        IRepositoryA repoA = _factory.GetRepository<IRepositoryA>();
        IRepositoryB repoB = _factory.GetRepository<IRepositoryB>();
        //Use repo A and B
    }

    public void SomeMethod2()
    {
        IRepositoryC repoC = _factory.GetRepository<IRepositoryC>();
        //Use repo C
    }

    //Other methods
}

This does solve the 2 problems above, but there are a few other issues:

  • RepositoryFactory is using IoC as a service locator, which is considered as anti-pattern by many people
  • RepositoryFactory has a dependency on IoC framework now, which might be undesirable in dependency injection (because as I understand, the application should have as little understanding about the IoC as they can, except for composition root, which is at the top of the application)

So is this a good approach to use?

4
  • Why not just use a container to create the dependency tree for you?
    – Sign
    Oct 10, 2014 at 13:19
  • 2
    Yes, it sounds like you need to split this class up into several, better-focused classes. Oct 10, 2014 at 13:42
  • @Sign my original approach is using IoC container to create the dependency tree, but as I mention the service class is growing more complicated, leading to some issues Oct 10, 2014 at 16:04
  • @MikePartridge Yes, I think so too, but in some cases, it might be difficult to split into multiple classes, e.g. many clients dependent on that, so I'm thinking of any work around Oct 10, 2014 at 16:04

2 Answers 2

4

Good Question!

When the service becomes complicated, the number of dependencies will grow more and more

In that case your dependencies are maybe not standalone dependencies but can be grouped into simple dependency-holding classes that can be used to pass into the constructor.

A good sign would be if more than one of your classes use both IRepositoryA an IRepositoryB in the constructor and also in some of their methods like in your SomeMethod1 method. You should be able to find a good name for the class that groups IRepositoryA and IRepositoryB. If not, consider going to tell us the real example so we can analyse better.

SomeMethod1() only uses repo A and B, but when client calls it, the service is created with all repositories, meaning it has more dependencies than it needs.

But that is okay because normally you want to make sure that at the time when the class is created you don't want the IRepositories to be changed anymore. They should be declared as immutable. That is a good practice.

If that's not the case and they shall be mutable, then you could make the IRepositoryC dependency beeing passed into SomeMethod2 as parameter. Currying / partial applying can help you to prevent code-duplication here when you call the method many times with the same IRepositoryC argument.

A different approach would be passing option-types into the constructor. Your class would then however have the responsibility to check in their methods if a given IRepository has already been provided or is still empty at the time of the method call.

So it depends a little on what you really want to do. There is imho no general answer to this problem.

2
  • thanks, I find it a bit strange to have some DTO/container to hold just repository reference, but yes I agree it's a good way to reduce number of parameters in constructor. What do you think about my 2nd approach? To be honest, I quite like the idea of the 2nd approach, since it looks clean to me, although there are some drawbacks as I mentioned Oct 10, 2014 at 16:09
  • It might not only just hold it. Imagine credentials. In a simple form, they consist only of a userId and a password. Same goes for a simple geo-coordinate(s) class. But they has to fit together by business logics / semantics. It's not good to put 2 things into a class just to pass only one argument. Actually your 2nd approach is exactly what I suggested but instead of putting together what fits together it is putting everything together. That hides what your method really needs to work and is like giving the method all 3 IRepository arguments (in grouped form) but it uses only few of them.
    – valenterry
    Oct 10, 2014 at 18:57
4

The main problem with your second approach of injecting factories is that it removes one of the benefits IoC/dependency injection provides - You no longer know which repositories the class depends upon without digging through the code.

While this is not as 'bad' as creating the repositories itself, the class still needs to know how to request them from a factory. Also, you've added extra complexity by introducing the factory itself.

You are probably be better off following your instinct that the class is doing to much and separating it out into smaller chunks of responsibility.

I completely agree with the answer provided by @valenterry it's like most things, ymmv and it really depends on what you want to do. The second approach isn't exactly evil and is perfectly servicable if that's what you end up with.

1
  • 1
    thanks, to me I prefer 2nd approach over constructor injection if not all constructor parameters used by all the methods in the class, maybe because in 2nd approach when I look at a specific method, I know what dependencies only that method uses, what dependencies shared by all methods. But yes I agree with you, maybe the cleanest way in this situation is to separate into several classes. Oct 10, 2014 at 16:17

Your Answer

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

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