Why not lump all service classes into a Factory method (instead of injecting interfaces)?

Question

We are building an ASP.NET project, and encapsulating all of our business logic in service classes. Some is in the domain objects, but generally those are rather anemic (due to the ORM we are using, that won't change). To better enable unit testing, we define interfaces for each service and utilize D.I.. E.g. here are a couple of the interfaces:

IEmployeeService
IDepartmentService
IOrderService
...

All of the methods in these services are basically groups of tasks, and the classes contain no private member variables (other than references to the dependent services). Before we worried about Unit Testing, we'd just declare all these classes as static and have them call each other directly. Now we'll set up the class like this if the service depends on other services:

public EmployeeService : IEmployeeService
{
   private readonly IOrderService _orderSvc;
   private readonly IDepartmentService _deptSvc;
   private readonly IEmployeeRepository _empRep;

   public EmployeeService(IOrderService orderSvc
                        , IDepartmentService deptSvc
                        , IEmployeeRepository empRep)
   {
      _orderSvc = orderSvc;
      _deptSvc = deptSvc;
      _empRep = empRep;
   }
   //methods down here
}

This really isn't usually a problem, but I wonder why not set up a factory class that we pass around instead?

i.e.

public ServiceFactory
{
   virtual IEmployeeService GetEmployeeService();
   virtual IDepartmentService GetDepartmentService();
   virtual IOrderService GetOrderService();
}

Then instead of calling:

_orderSvc.CalcOrderTotal(orderId) 

we'd call

_svcFactory.GetOrderService.CalcOrderTotal(orderid)

What's the downfall of this method? It's still testable, it still allows us to use D.I. (and handle external dependencies like database contexts and e-mail senders via D.I. within and outside the factory), and it eliminates a lot of D.I. setup and consolidates dependencies more.

Thanks for your thoughts!

Answer 1

One argument against this is that it doesn't make your dependencies clear. It shows that you depend on "some of the stuff in the service factory" but not which services. For refactoring purposes it can be helpful to know exactly what depends on what.

Dependency injection should make this kind of thing easy, if you're using an appropriate framework - it should just be a matter of creating the right constructor, defining what implements which interface, and letting it sort everything out.

Answer 2

If most of your classes depend on this three interfaces you could pass an object around that wraps them together, BUT: if most of the classes just depend on one or two of them then it's not a good idea since those classes will have access to objects they don't need and they have no business with and some programmers will always call the code they are not supposed to call just because it's available.

Btw, it's not a factory unless you always create a new object in the Get[...]Service() methods and doing that just for passing a few methods around is bad. I'd just call it ServiceWrapper and turn them into the properties EmployeeService, DepartmentService and OrderService.

Answer 3

Such a factory is essentially a Service Locator, and I consider it an anti-pattern because it obscures your dependencies and make it very easy to violate the Single Responsibility Principle (SRP).

One of the many excellent benefits we derive from Constructor Injection is that it makes violations of the SRP so glaringly obvious.