Gfrktyl

My team has developed a new service layer in our application. They created a bunch of services that implement their interfaces (E.g., ICustomerService, IUserService, etc). That's pretty good so far.

Here is where things get a bit strange: We have a class called "CoreService", which looks like this:

// ICoreService interface implements the interfaces of 

// all services in the system 100+ 

public class CoreService : ICoreService 

{

    // I don't like these lazy instance variables. I think they are pointless

    private readonly Lazy<CustomerService> _customerService; 

    private readonly Lazy<UserService> _userService;



    public CoreService()

    {

        // These violate the Dependency inversion principle. 

        // It also news up its dependencies, which is bad.

        _customerService = new CustomerService();

        _userService = new UserService();



        // And so forth

    }



    #region ICustomerService

    public long? GetCustomerCount()

    {

        return _customerService.GetCustomerCount();

    }

    #endregion



    #region IUserService

    public User GetUser(int userId, int customerId)

    {

        return _userService.GetUser(userId, customerId);

    }

    #endregion



    // ...

    // And 100 other regions for all services

}

The team's reasoning is that controllers in the consuming application can easily instantiate CoreService and use its services, and it wouldn't cause any performance problems since everything is "Lazy".

I tried to explain that this is a bad design because:

1. We are violating the Dependency Inversion Principle by lazy instantiating every single dependency and their dependencies.


2. As a result of #1, We are eliminating the testability of our services. We can no longer the mock dependency of our services and inject them for unit testing.


3. 
   CoreService just seems like a "God Object" anti-pattern to me.


4. We shouldn't even instantiate anything in our controllers. We should just inject the required dependencies of the controllers into it. (E.g., if the CustomerController requires five different services, just inject them via the constructor!)

Is my argument valid? Are there any other violations of best practices that I am missing here? Any input would be highly appreciated here.

Edit: I updated the title since this question is getting marked as a duplicate. This service is not necessarily a God object, it's actually a "Passthrough" or a Facade service. My apologies for the mistake.

This is not a God object.

It seems like it is because there is so much here, but, in a way, it's doing nothing at all. There is no behavior code here. This isn't an omnipotent God that does everything. It just finds everything. It's less a true object at all and more of a data structure.

This pattern has a more proper name: Service Locator. It strongly contrasts with the Dependency Injection^fowler pattern.

Your testablility complaint is valid but the problem is bigger than that. The main principle being violated here is the Interface Segregation principle.

Here's why: If I come at this thing with an intent to refactor away the Dependency Inversion problem I'd just stop hard coding CoreService. I'd replace it with a parameter that must be passed in. Can you see why that doesn't really help?

On top of it being annoying to see a CoreService parameter passed into everything, this doesn't actually fix the dependency problem because seeing that an object needs CoreService tells me zero about it's real needs because CoreService provides access to anything and everything. We externalize dependencies to make needs clear.

If we follow the Interface Segregation principle we see that an object that, right now, needs CoreService actually needs maybe 5 out of the 100 things it provides. What those 5 things need is a good descriptive name.

Now when I see that name I know what this thing needs. I can go find ways to provide those 5 things. There might be 2 or 42 ways to provide those 5 things. One of those ways might be through a test, but this idea is about a lot more than testing. Testing just makes this problem painfully obvious.

To provide that name, and avoid a constructor with 5 parameters, you can introduce a parameter object^{refactoring.com, c2.com} provided those 5 things represent one coherent idea with, please, a good name. (It may be 2 ideas that need 2 names are hiding in those 5 things. If so fine, break 'em up and name 'em).

You might be tempted to reuse this parameter object if you find another object that needs it. But let's say that object needs something else as well. So you add that something else to the parameter object. Even though the first object doesn't need this something else. Well we're now on our way to making a Service Locator again. You stop this by not accepting things you don't need. That's what the Interface Segregation principle is all about.

Another pattern hiding in here seems to be Singleton^c2.com. Dependency Injection has a wonderful alternative to that. Just build what you need in main once and pass references to what you built to everything that needs it. Once you have built your graph of long lived objects call one method on one of them to start the whole thing ticking. Just try not to go nuts. It's OK to break this up with factories and other creational patterns so long as you keep creation and behavior code separate.

To convince others you're going to have to start showing how this problem can be fixed. Make some things that express their needs and write code that satisfies those needs. A simple factory with a good name and it's own file to live in often gets this done. If you're stuck with the need to find the rest of their stuff let the factory deal with that problem. Now your object, including its class file, is blissfully unaware of their nonsense and won't care when you get around to fixing the rest of it.

It also news up its dependencies, which is bad.

That's a little simplistic. It's bad to "new up dependencies" inside the client object when you don't provide a way to override them. You seem to be using C#. C# has named arguments! Which means you can satisfy a dependency with an optional argument. Just be sure you have a good default value for it. Good default values should be chosen wisely. Don't use ones that surprise people.

Also, as @JimmyJames points out, brainlessly lazy loading everything is far from ideal. It doesn't actually speed things up. It only changes when you pay for it. It can make when you pay for it unpredictable and it can make configuration problems difficult to diagnose. It's essentially the caching problem. Widely recognized as one of the hardest problems in computer science (after giving things good names). So please don't use it thoughtlessly.

We have something else like an actual God Service.

The entire backend is accessed from the frontend by a single interface of 15 or so methods, all of which take Stream as argument (because it really is remoting to another process). Now, the stream API is ridiculous to code against, so we did that exactly once. The service object is this single object that has the same 15 methods and a few additional ones that boil down to transmutate arguments and call one of the others. The service object is constructed at will from the backend interface and an interface for an object describing the logged-in user.

The God Service appears to do everything. It actually does serialization of requests and deserialization of responses, for about 500 lines. The design philosophy of the methods is one method call is one transaction, and the client can make up an arbitrary large save transaction by passing an IEnumerable of objects to be saved.

The API actually has several implemenations, including two three different mocks for unit testing. The God Service has exactly one implementation and implements no interfaces.

I think the idea of having all these separate services is just silly, and creating and injecting all of them is even more silly. At least this God Service makes them all Lazy so it's much easier to deal with.

I have never seen a good design where swapping services one at a time at config level was a good idea. You always wanted to swap them in huge chunks (or more likely all at once) to one of a very few number of valid configurations, so there's little downside to this God Service design. It would be easily adapted to handling a which implementation of all the component services to load based on a single configuration entry. Since unnatural chimera configurations cannot exist anymore, it's less headache to support.

In simple words as I understood your problem... you have lots of internal services and then you have another external service which is being used to interactcalling all of these internal services.

In world of Onion Architecture we call internal services domain services and external services as application services. As shown in picture below,

Its always a bad practice to make calls to all internal services from a single external service.

The concept to take is that, externalapplication services are not bad as far as they logically make sense and not just a dump station for everything.

In case your domain services depend on each other, you can extract them into a third service naming it properly and put shared methods in them.