45

This question already has an answer here:

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.

marked as duplicate by gnat, Greg Burghardt, Robert Harvey, BobDalgleish, maple_shaft Dec 21 '18 at 20:04

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 27
    You are 9000% right. But you will never convince them. Just stay late and refactor everything while they are sleeping – Ewan Dec 20 '18 at 15:47
  • 7
    "Plus 1" for each of your concerns. – Jan Linxweiler Dec 20 '18 at 15:48
  • 12
    I'd add to your arguments that if all this does is just do pass through calls to the individual services, it probably doesn't have any reason to exist. Why would you want to update an extra class when a method signature changes? This is one where DRY needs to be applied. – Becuzz Dec 20 '18 at 15:54
  • 2
    @gnat Except for the fact that the first answer says "This is not a God object". – JimmyJames Dec 20 '18 at 20:00
  • 1
    @gnat I see getters, a canned caching mechanizm, and no indication that the 100 other regions do anything different. If there is actual logic in here I don't see it. – candied_orange Dec 20 '18 at 20:26
49

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 Injectionfowler 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 objectrefactoring.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 Singletonc2.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.

  • 4
    One disadvantage I don't think you covered is that the use of lazy-loading can create issues. To say it prevents performance problems seems a little backwards in my experience. It saves space iff you aren't using everything. But the first things to come along and ask could have to wait for a while and maybe error out. On a slightly different note, you also won't know about a configuration issue with those services until they are requested. Often you end up 'warming' things up which defeats the purpose. – JimmyJames Dec 20 '18 at 21:13
  • 1
    @JimmyJames better? – candied_orange Dec 21 '18 at 2:12
  • 1
    Your argument about creation and behavior being separate doesn't make much sense to me. Generally, creation is where most if not all of the complex behavior should happen, unless the object is intrinsically mutable. I feel that this philosophy nudges towards making more singletons and services than what is absolutely necessary. – Kafein Dec 21 '18 at 10:20
  • @Kafein I already provided one link about that. Here's another. Maybe you should post a question. – candied_orange Dec 21 '18 at 11:32
  • 1
    @Kafein - Even if the object is not intrinsically mutable, it may still have a boatload of complex behaviours inside - for instance, knowing how best to pass data from one source to another. If the object is stateless (or stateful, but immutable), the creation logic might be very simple but the internal logic very complex. – Sebastian Lenartowicz Dec 21 '18 at 12:26
2

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.

  • 2
    This even has a name, a facade – Izkata Dec 21 '18 at 15:37
0

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 interact\calling 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,

enter image description here

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

The concept to take is that, external\application 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.

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