Dependencies injection without constructor: really a bad practice?

Question

I'm working in a web solution with C#, MVC4, StructureMap, etc.

In the solution I have services for controllers. By example :

public class ServiceA{
    private readonly IRepository _repository1;
    private readonly IRepository _repository2;

    public ServiceA(IRepository1 repository1, IRepository2 repository2){
        _repository1=repository1;
        _repository2=repository2;
    }

    public void DoSomethingA(){
        _repository1.DoSomething();
    }

    public void DoSomethingB(){
        _repository2.DoSomething();
    }
}

public class ServiceB{
    private readonly IRepository _repository3;
    private readonly IRepository _repository4;

    public ServiceB(IRepository3 repository3, IRepository4 repository4){
        _repository3=repository3;
        _repository4=repository4;
    }

    public void DoSomethingA(){
        _repository3.DoSomething();
    }

    public void DoSomethingB(){
        _repository4.DoSomething();
    }
}

It is good practice to do this? :

public abstract class ServiceBase(){
    public IRepository1 Repository1 { get { return instanceOf<IRepository1>(); }}
    public IRepository2 Repository2 { get { return instanceOf<IRepository2>(); }}
    public IRepository3 Repository3 { get { return instanceOf<IRepository3>(); }}
    public IRepository4 Repository4 { get { return instanceOf<IRepository4>(); }}

    private T instanceOf<T>()
    {
        return ServiceLocator.Current.GetInstance<T>();
    }
}

And then create the services in this way?

public class ServiceA : ServiceBase
{
    public void DoSomethingA(){
        Repository1.DoSomething();
    }

    public void DoSomethingB(){
        Repository2.DoSomething();
    }
}


public class ServiceB : ServiceBase
{
    public void DoSomethingA(){
        Repository3.DoSomething();
    }
    public void DoSomethingB(){
        Repository4.DoSomething();
    }
}

With the second option I see certain advantages:

• It is not necessary to have a private variable for each repository.
• I do not need a constructor for services, making them smaller and easier to read.
• All repositories will be available in any service.
• The service does not get unnecessary instances. By example, calling in ServiceA the method DoSomethingA the ServiceLocator get only Repository1 instance. ( using the first method would receive two instances: for Repository1 and Repository2 )

In both cases I can make the appropriate tests:

• In the first case, sending the mocked object through the constructor.
• In the second case configuring StructureMap to use the mocked object when is necesary.

Do you think? I'm going against some principles? (sorry my english)

Answer 1

Lets look at the advantage arguments first:

It is not necessary to have a private variable for each repository.

That's correct. Although in reality those 4 bytes for the reference usually do not matter.

I do not need a constructor for services, making them smaller and easier to read.

I see it exactly the opposite way. Having a constructor tells you immediately what dependencies the class has. With the base class you have to look at the whole class to get that information. Also it makes it impossible to use tools to analyze if you have a good design with low coupling, high cohesion and no tangles. And those that are using the class have no clue at all what dependencies the class has unless they read the implementation.

All repositories will be available in any service.

It should be avoided to use many repos in one service since this increases coupling. Providing all repos to all services is the best way to encourage a bad design with a high coupling. So I see this as a disadvantage.

The service does not get unnecessary instances. By example, calling in ServiceA the method DoSomethingA the ServiceLocator get only Repository1 instance. ( using the first method would receive two instances: for Repository1 and Repository2 )

A service that uses completely different dependencies different methods is a huge indication that it doesn't follow the Single Responsibility Principle. Most likely it sould be split into two services in this case.

Regarding testability: in the second case by using a singleton (ServiceLocator) your test are not isolated anymore. So they can influence each other. Especially when run in parallel.

In my opinion you are on the wrong way. Using the Service Locator anti-pattern you are hiding the dependencies to those using your class, making it harder for those reading the implementation to see what dependencies the class has and your tests are not isolated anymore.

Answer 2

The problem I see with this design is that you are tightly coupling the ServiceLocator to your services, therefore are not promoting loose coupling. What happens if you want to unit test the service with mock/test repositories? Or even swap out your DI implementation. In practice it's probably not a big problem as you can configure your test services via the service locator, but it just feels code-smellish to me.