In a follow-up to his earlier post on Constructor Over-Injection, Jeffrey Palermo changes his stance on Constructor Over-Injection from anti-pattern to the more palatable code smell. In this post I introduce the concept of a Facade Service and outline a refactoring that addresses this code smell.

If I should extract a core message from Jeffrey Palermo's blog post it would be that it's a code smell if you have a class that takes too many dependencies in its constructor.

I can only agree, but only so far as it's a code smell. However, it has nothing to do with DI in general or Constructor Injection specifically. Rather, it's a smell that indicates a violation of the Single Responsibility Principle (SRP). Let's review the example constructor:

public OrderProcessor(IOrderValidator validator,
                      IOrderShipper shipper,
                      IAccountsReceivable receivable,
                      IRateExchange exchange,
                      IUserContext userContext)

In this version, I even added IOrderShipper back in as I described in my earlier post. Surely, five constructor parameters are too many.

Constructor Injection makes SRP violations glaringly obvious.

What's not to like? My personal threshold lies somewhere around 3-4 constructor parameters, so whenever I hit three, I start to consider if I could perhaps aggregate some of the dependencies into a new type.

I call such a type a Facade Service. It's closely related to Parameter Objects, but the main difference is that a Parameter Object only moves the parameters to a common root, while a Facade Service hides the aggregate behavior behind a new abstraction. While the Facade Service may start its life as a result of a pure mechanistic refactoring, it often turns out that the extracted behavior represents a Domain Concept in its own right. Congratulations: you've just move a little closer to adhering to the SRP!

Let's look at Jeffrey Palermo's OrderProcessor example. The core implementation of the class is reproduced here (recall that in my version, IOrderShipper is also an injected dependency):

public SuccessResult Process(Order order)
{
    bool isValid = _validator.Validate(order);
    if (isValid)
    {
        Collect(order);
        _shipper.Ship(order);
    }
 
    return CreateStatus(isValid);
}
 
private void Collect(Order order)
{
    User user = _userContext.GetCurrentUser();
    Price price = order.GetPrice(_exchange, _userContext);
    _receivable.Collect(user, price);
}

If you examine the code it should quickly become apparent that the Collect method encapsulates a cluster of dependencies: IAccountsReceivable, IRateExchange and IUserContext. In this case it's pretty obvious because they are already encapsulated in a single private method. In real production code, you may need to perform a series of internal refactorings before a pattern starts to emerge and you can extract an interface that aggregates several dependencies.

Now that we have identified the cluster of dependencies, we can extract an interface that closely resembles the Collect method:

public interface IOrderCollector
{
    void Collect(Order order);
}

In lieu of a better name, I simply chose to call it IOrderCollector, but what's interesting about extracting Facade Services is that over time, they often turn out to be previously implicit Domain Concepts that we have now dragged out in the open and made explicit.

We can now inject IOrderCollector into OrderProcessor and change the implementation of the private Collect method:

private void Collect(Order order)
{
    _collector.Collect(order);
}

Next, we can remove the redundant dependencies, leaving us with this constructor:

public OrderProcessor(IOrderValidator validator,
                      IOrderShipper shipper,
                      IOrderCollector collector)

With three constructor parameters it's much more acceptable, but we can always consider repeating the procedure and extract a new Facade Service that aggregates IOrderShipper and IOrderCollector.

The original behavior from the Collect method is still required, but is now implemented in the OrderCollector class:

public class OrderCollector : IOrderCollector
{
    private readonly IUserContext _userContext;
    private readonly IRateExchange _exchange;
    private readonly IAccountsReceivable _receivable;
 
    public OrderCollector(IAccountsReceivable receivable,
                          IRateExchange exchange,
                          IUserContext userContext)
    {
        _receivable = receivable;
        _exchange = exchange;
        _userContext = userContext;
    }
 
    #region IOrderCollector Members
 
    public void Collect(Order order)
    {
        User user = _userContext.GetCurrentUser();
        Price price = 
            order.GetPrice(_exchange, _userContext);
        _receivable.Collect(user, price);
    }
 
    #endregion
}

Here's another class with three constructor parameters, which falls within the reasonable range. However, once again, we can begin to consider whether the interaction between IUserContext and the Order could be better modeled.

In outline form, the Introduce Facade Service refactoring follows these steps:

  1. Analyze how dependencies interact to identify clusters of behavior.
  2. Extract an interface from these clusters.
  3. Copy the original implementation to a class that implements the new interface.
  4. Inject the new interface into the consumer.
  5. Replace the original implementation with a call the new dependency.
  6. Remove the redundant dependencies.
  7. Rinse and repeat :)

The beauty of Facade Services is that we can keep wrapping one Facade Service in new Facade Services to define more and more coarse-grained building blocks as we get closer and closer to the application boundary.

Keeping each class and its dependencies to simple interactions also makes it much easier to unit test all of them because none of them do anything particularly complex.

Adhering strictly to Constructor Injection makes it easy to see when one violates the SRP and should refactor to an Facade Service.

Update (2011.04.10): In my book I've changed the name of this concept to Facade Service as it more clearly communicates the relationship with the Facade pattern.

Last modified (2011.08.23): Changed all references to Aggregate Service (the old name of the concept) to Facade Service.


Comments

Arnis L. #
Had to review project of co-workers. Found exactly this anti-pattern and said - this is wrong but currently have no comments about how to fix that.

Now i have. Thanks. :)
2010-02-11 08:32 UTC
Sam #
Great post. Hell, great blog. Thank you for sharing :)
2010-04-09 15:29 UTC
Carl R #
I really like how you write!

I also just read Service locator is an anti pattern and I must say your posts are eye openers.
2011-05-03 19:24 UTC
(Looks like my last comment didn't make it...)
I have a question about that.
RAP states that you should have more than one implementation per interface.
How do you do this with Aggregate Services? In most cases, you will have only one, especially when refactoring...
2011-07-29 14:14 UTC
The Reused Abstractions Principle says that we should strive towards having more than one implementation of the same interface, and that we should consider 1:1 interfaces as code smells. That doesn't mean that 1:1 interfaces suddenly have become illegal - only that they warrant more investigation.

Even though I find the RAP extremely useful as a guide it doesn't mean that I ferociously stamp out every occurrence of 1:1 interfaces I find in my code bases. For example, when refactoring to Facade Services, I still think that this refactoring alone improves a code base. If that means that we (perhaps temporarily) end up with a 1:1 interface, I wouldn't be that concerned.

Still, in this blog post I outline what really is a ranking of composability patterns. More blog posts are going to follow on that topic, but I can already now reveal that the most composable (and thus most reusable) pattern is the Command pattern. Thus, the more Commands you have, the easier it will become to adhere to the RAP.

I believe that this is the underlying reason why so many people are reporting that CQRS are helping them to effectively deal with complexity.

A Facade Service can easily be expressed as a Command, so there's no conflict between Facade Services and the RAP.
2011-07-29 14:31 UTC
NightOwl888 #
Ok, either I have done something wrong, or I am interpreting what you are saying wrong. I have a business object - that is, an object that represents a domain entity. The object is responsible for maintaining state about the entity and references to its child entities, and as such it requires several services to 1) instantiate the internal state (collections and dictionaries) 2) plugins to provide alternate implementations of business logic, and 3) factories to provide abstract references to global resources (ambient context).

I started with 9 constructor parameters and got it down to 6 so far, but I am having trouble now because none of the services are grouped together in blocks like in this article and they don't seem all that related. Here is my constructor before:

public SiteMap(
ISiteMapBuilder siteMapBuilder,
IControllerTypeResolver controllerTypeResolver,
IActionMethodParameterResolver actionMethodParameterResolver,
IHttpContextFactory httpContextFactory,
IAclModule aclModule,
ISiteMapNodeCollectionFactory siteMapNodeCollectionFactory,
IGenericDictionaryFactory genericDictionaryFactory,
IUrlPath urlPath,
RouteCollection routes
)

And here it is now:


public SiteMap(
ISiteMapBuilder siteMapBuilder,
IMvcResolver mvcResolver,
IMvcContextFactory mvcContextFactory,
IAclModule aclModule,
ISiteMapChildStateFactory siteMapChildStateFactory,
IUrlPath urlPath
)

I renamed IHttpContextFactory because it has several types of context related (controller context, request context, http context) creation methods. I then simply added the routes to that factory so it can provide access to the global routes collection.

I created a new facade service that implements (and cascades to the original services) the methods from both IControllerTypeResolver and IActionMethodParameterResolver named IMvcResolver.

Then I created another facade ISiteMapChildStateFactory that implements and cascades the methods to ISiteMapNodeCollectionFactory and IGenericDictionaryFactory. This one feels a bit like a kludge - the only thing these have in common are that they are used to instantiate the object's internal collections. I feel like this is a step in the wrong direction.

Ironically, the entry point of the application is not much of a problem with only 5 dependencies and still room to divide up responsibilities, it is the business entity objects I am having problems with.

I am really having trouble finding more ways to combine these dependencies in logical ways. But perhaps I misunderstood - are you even talking about refactoring business entity objects here, or just command services that do not maintain their own state other than what is required to perform the action?

2013-02-24 18:55 UTC
You and your team are probably the only people who know what the SiteMap class and its dependencies do, so I can't tell you how to group them better. Could the SiteMap be taking on too many responsibilities?
2013-02-24 20:21 UTC
NightOwl888 #
The SiteMap is only responsible for maintaining its internal state, and it provides accessor methods that other objects use to indirectly manage the state and get information about its current state. Some of these accessor methods rely on services, but none of them rely on 2 or more services.

However, the IMvcResolver is only here to sync up the object's lifetime with the lifetime of the SiteMap - it is actually only used by external services, but it makes sense to create a single instance of it that services all of the SiteMap's node objects than to inject it directly into the nodes (not to mention, the situation with the node constructors is even worse). There can be more than one SiteMap instance in the application and the MvcResolver has an internal cache that needs to be in sync with the SiteMap's lifetime in case there are cache name collisions from one SiteMap to the next.

I can also tell you that ISiteMapBuilder, IMvcResolver, and IAclModule are all there to support alternate implementations (plugins) of logic. When we've run out of ways to group by domain concepts, in your opinion would it be wise to create a ISiteMapPluginService even though they are technically unrelated plugins? I think that could make sense if each service interface is exposed as a property of the ISiteMapPluginService rather than cascading the calls. Or would it make more sense to group by technology, say group all of the MVC services together?

2013-02-24 22:45 UTC
If "accessor methods rely on services, but none of them rely on 2 or more services" it sounds like the SiteMap class has poor cohesion, which again is a sign that it should be split into multiple smaller classes.
2013-02-25 07:20 UTC
NightOwl888 #
Poor cohesion? I thought it was a sign that I pulled out all of the possible facade services already - I considered it a good thing until you mentioned otherwise.

I started out with some 4000 lines of code and have whittled it down to 700 lines (including documentation and curly brackets). I am considering what you are saying about splitting this into more classes, but looking at the public interface and seeing all that is there are 1) add, remove, clear, find, and get methods that manage the internal state, 2) methods that pass child node object to another service to process or analyze it in some way or 3) are simple boolean or string properties that pertain to the whole object graph.

I can see potentially moving the find methods to an ISiteMapNodeFinder, but this would add constructor parameters, not remove them. I suppose I could potentially create a class that wraps the 4 internal dictionaries along with the accessor methods into a service, which would move the responsibility of instantiating the dictionaries to that service's constructor.

Anyway, I ended up taking the path I mentioned - just creating a ISiteMapPluginProvider that exposes service interfaces directly on its interface. Crude, but effective. It ended up making both the SiteMap class and the SiteMapNode class have very similar constructors, so I consider it a win.
2013-02-25 14:10 UTC


Wish to comment?

You can add a comment to this post by sending me a pull request. Alternatively, you can discuss this post on Twitter or somewhere else with a permalink. Ping me with the link, and I may respond.

Published

Tuesday, 02 February 2010 20:56:44 UTC

Tags



"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!
Published: Tuesday, 02 February 2010 20:56:44 UTC