Refactoring to Aggregate Services

Tuesday, 02 February 2010 20:56:44 UTC

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

AutoFixture 1.0

Wednesday, 27 January 2010 22:54:58 UTC

AutoFixture 1.0 is now available on the CodePlex site! Compared to Release Candidate 2 there are no changes.

The 1.0 release page has more details about this particular release, but essentially this is RC2 promoted to release status.

It's been almost a year since I started development on AutoFixture and I must say that it has been an exciting and fulfilling experience! The API has evolved, but has turned out to be surprisingly flexible, yet robust. I even had some positive surprises along the way as it dawned on me that I could do new fancy things I hadn't originally considered.

If you use the Likeness feature (of which I have yet to write), you will run into this bug in Visual Studio. The bug is only in IntelliSense, so any code using Likeness will compile and work just fine.

While this release marks the end of AutoFixture's initial days, it also marks the beginning of AutoFixture 2.0. I already have lots of plans for making it even more extensible and powerful, as well as plans for utility libraries that integrate with, say, Moq or Rhino Mocks. It's going to be an exciting new voyage!


Comments

Adrian Hara
Having gone through all the posts in the AutoFixture category, as well as the zero-friction TDD posts (first), all i can say is (paraphrasing http://en.wikipedia.org/wiki/Kenny_Bania) "This is Gold! Gold!". Thanks not only for the tools but for some amazing blog posts!

2010-02-08 10:39 UTC
Thanks for those kind words. They are much appreciated :)
2010-02-08 14:31 UTC
Ruddy lee
Good stuff!

Why called Ploeh?

I'm a speaker in MS Taiwan, try to know more about Ploeh.AutoFixture.dll

2010-03-07 02:51 UTC
Currently, the best place to learn about AutoFixture is this very blog. Apart from that, trying it out on some small, but real software project is always always the best way to truly learn a new technology.

If you have specific questions about AutoFixture, the best place to ask them would be in the AutoFixture discussion forum.

HTH
2010-03-07 09:17 UTC

What's so dangerous about a DI attribute?

Wednesday, 27 January 2010 19:36:34 UTC

In a reaction to Uncle Bob's recent post on Dependency Injection Inversion, Colin Decker writes that he doesn't consider the use of the single Guice @Inject annotation particularly problematic. As I read it, the central argument is that

annotations are not code. By themselves, they do nothing.

I'll have to take that at face value, but if we translate this reasoning to .NET it certainly holds true. Attributes don't do anything by themselves.

I'm not aware of any DI Container for .NET that requires us to sprinkle attributes all over our code to work (I don't consider MEF a DI Container), but for the sake of argument, let's assume that such a container exists (let's call it Needle). Would it be so bad if we had to liberally apply the Needle [Inject] attribute in large parts of our code bases?

Colin suggests no. As usual, my position is that it depends, but in most cases I would consider it bad.

If Needle is implemented like most libraries, InjectAttribute is just one of many types that make up the entire API. Other types would include NeedleContainer and its associated classes.

Java annotations may work differently, but in .NET we need to reference a library to apply one of its attributes. To apply the [Inject] attribute, we would have to reference Needle, and herein lies the problem.

Once Needle is referenced, it becomes much easier for a junior developer to accidentally start directly using other parts of the Needle API. Particularly he or she may start using Needle as a Service Locator. When that happens, Needle is no longer a passive participant of the code, but a very active one, and it becomes much harder to separate the code from the Container.

To paraphrase Uncle Bob: I don't want to write a Needle application.

We can't even protect ourselves from accidental usage by writing a convention-based unit test that fails if Needle is referenced by our code, because it must be referenced for the [Inject] attribute to be applied.

The point is that the attribute drags in a reference to the entire container, which in my opinion is a bad thing.

So when would it be less problematic?

If Needle was implemented in such a way that InjectAttribute was defined in an assembly that only contains that one type, and the rest of Needle was implemented in a different assembly, the attribute wouldn't drag the rest of the container along.

Whether this whole analysis makes sense at all in Java, and whether Guice is implemented like that, I can't say, but in most cases I would consider even a single attribute to be unacceptable pollution of my code base.


Comments

In addition to its own annotations, Guice supports the atinject API. This minimal 6-class package contains only the stuff you need to import in your application code and nothing you don't. These are standard annotations supported by Guice and other Java dependency injection frameworks.
http://code.google.com/p/atinject/
http://atinject.googlecode.com/svn/tags/1/javadoc/javax/inject/package-summary.html
2010-01-28 03:08 UTC
Ninject does it's injection magic using an [Inject] attribute. I agree with you that sprinkling your code base with those attributes is problematic. Not only because of the direct dependency, but this switching IoC frameworks becomes much harder. The Common Service Locator project defines an interface for IoC implementations. This allows you to swap implementations. However, when your code is polluted with those attribute declarations, switching isn't easy anymore.
2010-02-04 15:09 UTC
I'm no expert in Ninject, but last time I checked, the [Inject] attribute was optional.
2010-02-04 15:41 UTC

IWindsorInstaller

Tuesday, 26 January 2010 19:24:51 UTC

Reacting to my previous post, Krzysztof Koźmic was so kind to point out to me that I really should be using an IWindsorInstaller instead of writing the registration code in a static helper method (it did make me cringe a bit).

As it turns out, IWindsorInstaller is not a particularly well-described feature of Castle Windsor, so here's a quick introduction. Fortunately, it is very easy to understand.

The idea is simply to package configuration code in reusable modules (just like the Guice modules from Uncle Bob's post).

Refactoring the bootstrap code from my previous post, I can now move all the container configuration code into a reusable module:

public class BillingContainerInstaller : IWindsorInstaller
{
    #region IWindsorInstaller Members
 
    public void Install(IWindsorContainer container,
        IConfigurationStore store)
    {
        container.AddComponent<TransactionLog, 
            DatabaseTransactionLog>();
        container.AddComponent<CreditCardProcessor,
            MyCreditCardProcessor>();
        container.AddComponent<BillingService>();
    }
 
    #endregion
}

While I was at it, I also changed from the fluent registration API to the generic registration methods as I didn't really need the full API, but I could have left it as it was.

BillingContainerInstaller implements IWindsorInstaller, and I can now configure my container instance like this:

var container = new WindsorContainer();
container.Install(new BillingContainerInstaller());

The Install method takes a parameter array of IWindsorInstaller instances, so you can pass as many as you'd like.


Comments

Thomas
It seemes that this feature is something like Registry in StructureMap?
2010-01-26 21:35 UTC
I have to admit that I'm not yet entirely up to speed on all the different DI Containers out there, but it's a common feature that several DI Containers have.
2010-01-26 22:04 UTC
This may be trivial, but remember to implement a ReleasePolicy when using transient objects. Otherwise you will use an unheard amount of RAM to hold all instances created, because .Release() will not be called on each one of them.
2010-01-27 07:14 UTC
Thanks for pointing that out - did you have this particular blog post in mind?

I'll have to investigate whether that still holds true. If so, I would be inclined to consider it a bug on Windsor's part, but it may be by design, influenced by considerations I simply have yet to realize.
2010-01-27 08:17 UTC
Windsor committer Krzysztof Koźmic was so kind to investigate this, and he left a comment at the blog post to the effect that this was a bug in Windsor. It is now fixed in Windsor 2.1, so having dependencies implement IDisposable should be enough.
2010-01-27 09:02 UTC
urkurk
hi mark,

was wondering where i could find a sample application that uses castle windsor and asp.net webforms?
2010-04-04 09:52 UTC
Hi urkurk

Thank you for writing. Unfortunately, I'm not aware of such a sample application. Have you tried asking on the Castle Windsor forum?
2010-04-04 14:49 UTC
The system is not modular when you have to explicitly instanciate the BillingContainerInstaller on your code and pass the instance to the install method, I haven't investigate on the IWindsorInstaller yet, but I'm hoping that Windsor during the normal registration process of components canintercept those that implement that interface, and automatically call the Install method.

This sounds very similar to the startable facility though, with the important difference that in this case you have some more arguments.
2010-09-22 09:25 UTC
AFAIR the new version of Castle Windsor (2.5) enables you to specify installers in app.config.
2010-09-22 09:47 UTC
Sasireka
Hi,

I am new to castle windsor. Could you please tell me the difference between container.AddComponent(...) & container.Register(component.For<...>..).

When we need to use AddComponent() / Register()?

Thanks in advance !
2011-08-11 09:39 UTC
AFAIK AddComponent is a legacy method and will be removed in future versions of Castle Windsor, but that I didn't know when I wrote that specific example :$
2011-08-11 09:44 UTC
Sasireka
Hi Mark,

Thank you for your answer. I have an another clarification. What are the advantages offer the castle windsor framework rather than using the normal coding.

2011-08-12 05:34 UTC
The same advantage as any other DI Container.

In addition to that, Castle Windsor (and Unity and Spring.NET) also supports interception.
2011-08-12 06:44 UTC
sasireka
Hi Mark,

I need another help about castle windsor.

Could you please tell about how to check whether the particular assembly registered or not in the castle container.

Actually i am using the below code for register the assembly. i need to avoid the registering process, if it was already registered.

container.Register(AllTypes.FromAssemblyNamed(pAssemblyName).Pick().WithService.DefaultInterface());
Thanks in advance.
2011-09-19 09:43 UTC
I can't recall off the top of my head - may I suggest that you ask on Stack Overflow?
2011-09-19 10:12 UTC

Dependency Injection Inversion in .NET

Monday, 25 January 2010 20:48:27 UTC

About a week ago Uncle Bob published a post on Dependency Injection Inversion that caused quite a stir in the tiny part of the .NET community I usually pretend to hang out with. Twitter was alive with much debate, but Ayende seems to sum up the .NET DI community's sentiment pretty well:

if this is a typical example of IoC usage in the Java world, then [Uncle Bob] should peek over the fence to see how IoC is commonly implemented in the .Net space

Despite having initially left a more or less positive note to Uncle Bob's post, after having re-read it carefully, I am beginning to think the same, but instead of just telling everyone how much greener the grass is on the .NET side, let me show you.

First of all, let's translate Uncle Bob's BillingService to C#:

public class BillingService
{
    private readonly CreditCardProcessor processor;
    private readonly TransactionLog transactionLog;
 
    public BillingService(CreditCardProcessor processor,
        TransactionLog transactionLog)
    {
        if (processor == null)
        {
            throw new ArgumentNullException("processor");
        }
        if (transactionLog == null)
        {
            throw new ArgumentNullException("transactionLog");
        }
 
        this.processor = processor;
        this.transactionLog = transactionLog;
    }
 
    public void ProcessCharge(int amount, string id)
    {
        var approval = this.processor.Approve(amount, id);
        this.transactionLog.Log(string.Format(
            "Transaction by {0} for {1} {2}", id, amount, 
            this.GetApprovalCode(approval)));
    }
 
    private string GetApprovalCode(bool approval)
    {
        return approval ? "approved" : "denied";
    }
}

It's nice how easy it is to translate Java code to C#, but apart from casing and other minor deviations, let's focus on the main difference. I've added Guard Clauses to protect the injected dependencies against null values as I consider this an essential and required part of Constructor Injection - I think Uncle Bob should have added those as well, but he might have omitted them for brevity.

If you disregard the Guard Clauses, the C# version is a logical line of code shorter than the Java version because it has no DI attribute like Guice's @Inject.

Does this mean that we can't do DI with the C# version of BillingService? Uncle Bob seems to imply that we can do Dependency Inversion, but not Dependency Injection - or is it the other way around? I can't really make head or tails of that part of the post…

The interesting part is that in .NET, there's no difference! We can use DI Containers with the BillingService without sprinkling DI attributes all over our code base. The BillingService class has no reference to any DI Container.

It does, however, use the central DI pattern Constructor Injection. .NET DI Containers know all about this pattern, and with .NET's static type system they know all they need to know to wire dependencies up correctly. (I thought that Java had a static type system as well, but perhaps I am mistaken.) The .NET DI Containers will figure it out for you - you don't have to explicitly tell them how to invoke a constructor with two parameters.

We can write an entire application by using Constructor Injection and stacking dependencies without ever referencing a container!

Like the Lean concept of the Last Responsible Moment, we can wait until the application's entry point to decide how we will wire up the dependencies.

As Uncle Bob suggests, we can use Poor Man's DI and manually create the dependencies directly in Main, but as Ayende correctly observes, that only looks like an attractive alternative because the example is so simple. For complex dependency graphs, a DI Container is a much better choice.

With the C# version of BillingService, which DI Container must we select?

It doesn't matter: we can choose whichever one we would like because we have been following patterns instead of using a framework.

Here's an example of an implementation of Main using Castle Windsor:

public static void Main(string[] args)
{
    var container = new WindsorContainer();
    Program.Configure(container);
 
    var billingService =
        container.Resolve<BillingService>();
    billingService.ProcessCharge(2034, "Bob");
}

This looks a lot like Uncle Bob's first Guice example, but instead of injecting a BillingModule into the container, we can configure it inline or in a helper method:

private static void Configure(WindsorContainer container)
{
    container.Register(Component
        .For<TransactionLog>()
        .ImplementedBy<DatabaseTransactionLog>());
    container.Register(Component
        .For<CreditCardProcessor>()
        .ImplementedBy<MyCreditCardProcessor>());
    container.Register(Component.For<BillingService>());
}

This corresponds more or less to the Guice-specific BillingModule, although Windsor also requires us to register the concrete BillingService as a component (this last step varies a bit from DI Container to DI Container - it is, for example, redundant in Unity).

Imagine that in the future we want to rewire this program to use a different DI Container. The only piece of code we need to change is this Composition Root. We need to change the container declaration and configuration and then we are ready to use a different DI Container.

The bottom line is that Uncle Bob's Dependency Injection Inversion is redundant in .NET. Just use a few well-known design patterns and principles and you can write entire applications with DI-friendly, DI-agnostic code bases.

I recently posted a first take on guidelines for writing DI-agnostic code. I plan to evolve these guiding principles and make them a part of my upcoming book.


Comments

I don't agree with your implication that Uncle Bob's views are justified in the context of Java's DI frameworks... they are not. He appears to understand neither how DI frameworks in general are supposed to be used nor all the advantages they provide. That's where the real problem lies, not in the differences between .NET and Java frameworks.

The *only* difference between the example you give here and how Guice is used is that in Guice you'd annotate the constructor with @Inject, and I don't really buy there being any significant disadvantage to doing so in most cases. I posted in a bit more detail why I don't think the use of @Inject is a problem on my blog yesterday.
2010-01-26 22:18 UTC
Thanks for your comment.

If my previous post left the impression that I find Uncle Bob's views justified in the context of Java's DI Containers, that was accidental: I have no opinion on whether or not this is the case as I know too little about the detailed mechanics of Java and the DI Containers available there to have an opinion either for or against.

Whether or not a single annotation constitutes a problem or not ended up warranting an entire new blog post in response :)
2010-01-27 19:49 UTC
Ricardo Lacerda Castelo Branco
Mark,

I think your code still has the same problem that "Uncle Bob" was trying to solve. If you want a new instance of BillingService from deep in the bowels of the application, you need to reference the container! So, you are creating a dependency on WindsorContainer.

Maybe the solution would be program to an interface instead of depend on a concrete container.

What do you think?
2010-02-01 02:24 UTC
That is actually a different issue, but I agree that if we need to take a dependency on WindsorContainer or any other particular DI Container, we haven't solved the problem.

However, explaining how to deal with that will require another blog post - watch this space the next couple of days.
2010-02-01 05:47 UTC
How do we deal with the issue if we need BillingService deep in the bowels of the application?

Well, take a dependency on BillingService using standard Constructor Injection. Does this mean that we should have to pass an instance of BillingService through all constructors on the way down? No, rather not.

The answer lies in Aggregate Services. Instead of taking dependencies with the only purpose of passing them on to other dependencies, we can define a more coarse-grained service that encapsulates the desired behavior. This is exactly the kind of scenario where DI Containers excel, because they are able to auto-wire complex object graphs based entirely on their knowledge of which dependencies are required by each concrete type. They can do that when you use Constructor Injection because this knowledge is statically encoded in the type.

Then what if you need BillingService both deep in the bowels of the application as well as near the surface? In most cases I would say that this is a design smell that indicates that the granularity of BillingService is poorly defined. A dependency should be either a fine-grained 'leaf' dependency, or an Aggregate Service - not both.
2010-02-03 10:03 UTC
Thank You for your great contributions to the community
2011-02-06 11:39 UTC

AutoFixture 1.0 RC2

Wednesday, 20 January 2010 22:59:39 UTC

AutoFixture 1.0 Release Candidate 2 is now available on the CodePlex site! Compared to Release Candidate 1 there are very few changes, but the test period uncovered the need for a few extra methods on a recent addition to the library. RC2 contains these additional methods.

This resets the clock for the Release Candidate trial period. Key users have a chance to veto this release until a week from now. If no-one complains within that period, we will promote RC2 to version 1.0.

The RC2 release page has more details about this particular release.


Enabling DI for Lazy Components

Wednesday, 20 January 2010 18:08:36 UTC

My previous post led to this comment by Phil Haack:

Your LazyOrderShipper directly instantiates an OrderShipper. What about the dependencies that OrderShipper might require? What if those dependencies are costly?

I didn't want to make my original example more complex than necessary to get the point across, so I admit that I made it a bit simpler than I might have liked. However, the issue is easily solved by enabling DI for the LazyOrderShipper itself.

As always, when the dependency's lifetime may be shorter than the consumer, the solution is to inject (via the constructor!) an Abstract Factory, as this modification of LazyOrderShipper shows:

public class LazyOrderShipper2 : IOrderShipper
{
    private readonly IOrderShipperFactory factory;
    private IOrderShipper shipper;
 
    public LazyOrderShipper2(IOrderShipperFactory factory)
    {
        if (factory == null)
        {
            throw new ArgumentNullException("factory");
        }
 
        this.factory = factory;
    }
 
    #region IOrderShipper Members
 
    public void Ship(Order order)
    {
        if (this.shipper == null)
        {
            this.shipper = this.factory.Create();
        }
        this.shipper.Ship(order);
    }
 
    #endregion
}

But, doesn't that reintroduce the OrderShipperFactory that I earlier claimed was a bad design?

No, it doesn't, because this IOrderShipperFactory doesn't rely on static configuration. The other point is that while we do have an IOrderShipperFactory, the original design of OrderProcessor is unchanged (and thus blissfully unaware of the existence of this Abstract Factory).

The lifetime of the various dependencies is completely decoupled from the components themselves, and this is as it should be with DI.

This version of LazyOrderShipper is more reusable because it doesn't rely on any particular implementation of OrderShipper - it can Lazily create any IOrderShipper.


Comments

We could indicate optional dependencies by using Func<TResult> - http://msdn.microsoft.com/en-us/library/bb534960.aspx - delegates for the given constructor parameter. The given DI framework could be configured to resolve any Func<TResult> as x => Container.Resolve<TResult>()

This way the lazy/optional nature of the parameter is obvious to clients of the class, and there is no need to generate lazy implementation classes manually.

Note: I haven't had any practical experience using DI frameworks, so the above might not be possible at all :)
2010-01-20 20:44 UTC
Yes, Func<T> is sometimes a viable option. In general, I consider delegates to be anonymous interfaces, so Func<T> is really just an Abstract Factory. In other words, IOrderShipperFactory is functionally equivalent to Func<IOrderShipper>.

I had a period where I used a lot of delegates as injected dependencies, but I have more or less abandonded that approach again. While it technically works fine, it makes unit testing a bit harder because it's harder to test that a given object contains a specific type of Strategy if it's just a Func<T> or similar.

In any case, I'm mostly familiar with Castle Windsor at the moment. Although I have yet to try it out, I think the new Typed Factory Facility fits the bill very well - with that, we would never have to code a real implementation of IOrderShipperFactory because Windsor would be able to dynamically emit one for us.
2010-01-20 21:56 UTC
I have the feeling I did not set the context. Let me do that, and tell me if I the issues you raised still hold - both of them are important!

What I meant to propose is that we change Jeffrey Palermo's original example like the below:
- private readonly IOrderShipper _shipper;
+ private readonly Func< IOrderShipper > _shipperFactory;


- public OrderProcessor(IOrderValidator validator, IOrderShipper shipper)
+ public OrderProcessor(IOrderValidator validator, Func< IOrderShipper > shipperFactory)

- _shipper = shipper;
+ _shipperFactory = shipperFactory;

- _shipper.Ship(order);
+ _shipperFactory().Ship(order);

The change to the tests should be straightforward as well,

- new OrderProcessor(validator, mockShipper)
+ new OrderProcessor(validator, () => mockShipper)
2010-01-20 22:29 UTC
I'm surprised no one has mentioned .NET 4's Lazy<T>.

To communicate intent, it's clearer than Func<T>:

public UsesOrderShipper(Lazy<IOrderShipper> orderShipper)

There's a more complete example using Lazy<T> with Autofac.

Cheers,
Nick
2010-01-21 10:59 UTC
To be fair, Alwin mentioned it over on Jeffrey Palermo's original post before I posted my response.

That would definitely be an option as well, but I rather wanted to show the route involving absolutely no redesign of the original OrderProcess, and I couldn't do that purely with Lazy<IOrderShipper>. The most important point I wanted to make is that you can solve this problem using basic tools available since .NET 1.0.

It would, however, make a lot of sense to implement LazyOrderShipper by injecting Lazy<IOrderShipper> into it instead of inventing a new IOrderShipperFactory interface.
2010-01-21 17:47 UTC
I like the Func&lt;T&gt; and Lazy&lt;T&gt; solutions for addressing any real performance issues, but based upon the original example I still submit that the cleanest approach would be to just register the type with a singleton lifestyle to begin with. After the first two valid orders, it's more efficient.
2010-01-22 15:25 UTC
@Derek Greer: Aggreed, and that was also my initial point in my previous post.
2010-01-22 23:54 UTC
Thomas
Mark,

While it's easy to get it work with Typed Factory Facility and Castle, how do you implement the factory :

- without static configuration ?
- without passing the container in ?

Or I missed something ?

Thanks,

Thomas
2012-03-14 21:52 UTC
Thomas, who said anything about a DI Container or Castle Windsor for that matter?
2012-03-15 06:47 UTC
Thomas
Mark,

I was refering to your first comment. If I have no problem with the pattern I would like to know how you would do from the implementation point of view.

Thanks,

Thomas
2012-03-15 08:50 UTC
Ah, sorry... I'm not sure I entirely understand the question. With Windsor's Typed Factory Facility, you'd reqister the IOrderShipperFactory as being auto-generated by Windsor. I can't recall the exact syntax for this right now, but that registration would happen as part of the Registration phase of RRR.
2012-03-15 09:26 UTC
Thomas
Mark,

With Windsor there is no problem as TypedFactoryFacility provides implementation on the fly. However if you take another container you have to provide the implementation of IOrderShipperFactory on your own. Now the question is. How my implementation of the factory will pull the IOrderShipper implementation from the container ? I see two choices :

- configure staticaly (like Jeffrey did in his post)
- pass the container into the factory that it could resolve IOrderShipper.
- third choice that I don't know :)

I hope it's clearer now. Let me know if it doesn't make sense.

Thanks,

Thomas
2012-03-15 10:04 UTC
Thomas, I wrote a new blog post to answer your question.

HTH
2012-03-15 21:04 UTC
Thomas
Thanks Mark, I go to read it :)
2012-03-16 14:11 UTC

Rebuttal: Constructor over-injection anti-pattern

Wednesday, 20 January 2010 16:28:03 UTC

Jeffrey Palermo recently posted a blog post titled Constructor over-injection anti-pattern - go read his post first if you want to be able to follow my arguments.

His point seems to be that Constructor Injection can be an anti-pattern if applied too much, particularly if a consumer doesn't need a particular dependency in the majority of cases.

The problem is illustrated in this little code snippet:

bool isValid = _validator.Validate(order);  
if (isValid) 
{
    _shipper.Ship(order);
}

If the Validate method returns false often, the shipper dependency is never needed.

This, he argues, can lead to inefficiencies if the dependency is costly to create. It's not a good thing to require a costly dependency if you are not going to use it in a lot of cases.

That sounds like a reasonable statement, but is it really? And is the proposed solution a good solution?

No, this isn't a reasonable statement, and the proposed solution isn't a good solution.

It would seem like there's a problem with Constructor Injection, but in reality the problem is that it is being used incorrectly and in too constrained a way.

The proposed solution is problematic because it involves tightly coupling the code to OrderShipperFactory. This is more or less a specialized application of the Service Locator anti-pattern.

Consumers of OrderProcessor have no static type information to warn them that they need to configure the OrderShipperFactory.CreationClosure static member - a completely unrelated type. This may technically work, but creates a very developer-unfriendly API. IntelliSense isn't going to be of much help here, because when you want to create an instance of OrderProcessor, it's not going to remind you that you need to statically configure OrderShipperFactory first. Enter lots of run-time exceptions.

Another issue is that he allows a concrete implementation of an interface to change the design of the OrderProcessor class - that's hardly in the spirit of the Liskov Substitution Principle. I consider this a strong design smell.

One of the commenters (Alwin) suggests instead injecting an IOrderShipperFactory. While this is a better option, it still suffers from letting a concrete implementation influence the design, but there's a better solution.

First of all we should realize that the whole case is a bit construed because although the IOrderShipper implementation may be expensive to create, there's no need to create a new instance for every OrderProcessor. Instead, we can use the so-called Singleton lifetime style where we share or reuse a single IOrderShipper instance between multiple OrderProcessor instances.

The beauty of this is that we can wait making that decision until we wire up the actual dependencies. If we have implementations of IOrderShipper that are inexpensive to create, we may still decide to create a new instance every time.

There may still be a corner case where a shared instance doesn't work for a particular implementation (perhaps because it's not thread-safe). In such cases, we can use Lazy loading to create a LazyOrderShipper like this (for clarity I've omitted making this implementation thread-safe, but that would be trivial to do):

public class LazyOrderShipper : IOrderShipper
{
    private OrderShipper shipper;
 
    #region IOrderShipper Members
 
    public void Ship(Order order)
    {
        if (this.shipper == null)
        {
            this.shipper = new OrderShipper();
        }
        this.shipper.Ship(order);
    }
 
    #endregion
}

Notice that this implementation of IOrderShipper only creates the expensive OrderShipper instance when it needs it.

Instead of directly injecting the expensive OrderShipper instance directly into OrderProcessor, we wrap it in the LazyOrderShipper class and inject that instead. The following test proves the point:

[TestMethod]
public void OrderProcessorIsFast()
{
    // Fixture setup
    var stopwatch = new Stopwatch();
    stopwatch.Start();
 
    var order = new Order();
 
    var validator = new Mock<IOrderValidator>();
    validator.Setup(v => 
        v.Validate(order)).Returns(false);
 
    var shipper = new LazyOrderShipper();
 
    var sut = new OrderProcessor(validator.Object,
        shipper);
    // Exercise system
    sut.Process(order);
    // Verify outcome
    stopwatch.Stop();
    Assert.IsTrue(stopwatch.Elapsed < 
        TimeSpan.FromMilliseconds(777));
    Console.WriteLine(stopwatch.Elapsed);
    // Teardown
}

This test is significantly faster than 777 milliseconds because the OrderShipper never comes into play. In fact, the stopwatch instance reports that the elapsed time was around 3 ms!

The bottom line is that Constructor Injection is not an anti-pattern. On the contrary, it is the most powerful DI pattern available, and you should think twice before deviating from it.


Comments

Thanks for publishing this response. When I read the post yesterday evening, I had a lot of thoughts I was chewing on, and you tackled most of them. I couldn't tell if the "smell" was that the dependency was only sometimes needed, or that the dependency was only sometimes needed, and was expensive to create.

It seemed like the need to create a set of factories and have them make calls into the IOC container directly was a bigger smell. And the fact that the OrderShipper would be comming from a factory setup somewhere else, making code maintenance more difficult, and the application more prone to errors.

Decorating the implementation with a lazy load implementation is very clever. I hadn't thought of that solution, but the singleton lifecycle had.

Well done.
2010-01-20 16:50 UTC
I like this approach, but you've also made a contrived example. Your LazyOrderShipper directly instantiates an OrderShipper. What about the dependencies that OrderShipper might require? What if those dependencies are costly? Is it lazy turtles all the way down?
2010-01-20 17:14 UTC
Constructor over-injection is the problem. I rather like constructor injection
2010-01-20 17:20 UTC
Thank you all for your comments.

The point raised by Phil Haack is valid, so I've addressed it in a second blog post.
2010-01-20 18:09 UTC
SP
Maybe I'm missing something, but why isn't LazyOrderShipper thread safe as it stands? Your shipper member variable isn't static.
2010-01-20 18:44 UTC
Why isn't LazyOrderShipper thread-safe as it is?

When I wrote that, my thinking was that if the Ship method is hit by two or more threads simultaneously, more than one thread may evaluate the member field to null and start creating a new instance of OrderShipper. At least one of those instance will created, but never permanently assigned, since another thread may overwrite the value.

In this case, it may not lead to any exceptions (that I can think of), but it's inefficient use of resources.

However, every time I post something about threading on this blog, someone will point out my errors, so I'm looking forward to learn which mistake I made this time around :)

I'm no threading expert, in case you were wondering :)
2010-01-20 19:00 UTC
SP
As yes, your right -- I was thinking the calling code could lock against the object itself, but you would never know when LazyOrderShipper was writing to the property. (I'm not threading guru by any means, either).

Ok, back to DI... :-)
2010-01-20 19:35 UTC
Thomas
Great post as well as your book
2010-01-20 20:21 UTC
This would be a good solution if the problem wasn't so contrived. I've never run into a scenario where it would take me seven seconds to instantiate an object. It might take me 7 seconds to execute a method, but never instantiate the object itself.

My point is: It is like arguing that my unicorn needs a better saddle, and that saddle will let me ride it better.

I commend you anyways for your effort.
2010-01-22 21:52 UTC
I agree, but I just couldn't let the claim that Constructor Injection is an anti-pattern go by without reacting.
2010-01-22 23:56 UTC

AutoFixture 1.0 RC1

Wednesday, 13 January 2010 22:48:13 UTC

AutoFixture 1.0 Release Candidate 1 is now available on the CodePlex site! AutoFixture is now almost ten months old and has seen extensive use internally in Safewhere during most of this time. It has proven to be very stable, expressive and generally a delight to use.

If all goes well, the Release Candidate period will be short. Key users have a chance to veto the this version, but if no-one complains within a week from now, we will promote RC1 to version 1.0.

The RC1 release page has more detail about this particular release.


Anonymous Get

Monday, 04 January 2010 20:53:24 UTC

In a previous post I described how AutoFixture's Do method can let you invoke commands on your SUT with Dummy values for the parameters you don't care about.

The Get method is the equivalent method you can use when the member you are invoking returns a value. In other words: if you want to call a method on your SUT to get a value, but you don't want the hassle of coming up with values you don't care about, you can let the Get method supply those values for you.

In today's example I will demonstrate a unit test that verifies the behavior of a custom ASP.NET MVC ModelBinder. If you don't know anything about ASP.NET MVC it doesn't really matter. The point is that a ModelBinder must implement the IModelBinder interface that defines a single method:

object BindModel(ControllerContext controllerContext,
    ModelBindingContext bindingContext);

In many cases we don't care about one or the other of these parameters, but we still need to supply them when unit testing.

The example is a bit more complex than some of my other sample code, but once in a while I like to provide you with slightly more realistic AutoFixture examples. Still, it's only 10 lines of code, but it looks like a lot more because I have wrapped several of the lines so that the code is still readable on small screens.

[TestMethod]
public void BindModelWillReturnCorrectResult()
{
    // Fixture setup
    var fixture = new Fixture();
    fixture.Customize<ControllerContext>(ob =>
        ob.OmitAutoProperties());
 
    var value = fixture.CreateAnonymous("Value");
    var bindingContext = new ModelBindingContext();
    bindingContext.ValueProvider = 
        new Dictionary<string, ValueProviderResult>();
    bindingContext.ValueProvider["MyValue"] = 
        new ValueProviderResult(value, value, 
            CultureInfo.CurrentCulture);
 
    var expectedResult = 
        new string(value.Reverse().ToArray());
    var sut = fixture.CreateAnonymous<MyModelBinder>();
    // Exercise system
    var result = fixture.Get((ControllerContext cc) =>
        sut.BindModel(cc, bindingContext));
    // Verify outcome
    Assert.AreEqual(expectedResult, result, "BindModel");
    // Teardown
}

The first part simply creates the Fixture object and customizes it to disable AutoProperties for all ControllerContext instances (otherwise we would have to set up a lot of Test Doubles for such properties as HttpContext, RequestContext etc.).

The next part of the test sets up a ModelBindingContext instance that will be used to exercise the SUT. In this test case, the bindingContext parameter of the BindModel method is important, so I explicitly set that up. On the other hand, I don't care about the controllerContext parameter in this test case, so I ask the Get method to take care of that for me:

var result = fixture.Get((ControllerContext cc) =>
    sut.BindModel(cc, bindingContext));

The Get method creates a Dummy value for the ControllerContext, whereas I can still use the outer variable bindingContext to call the BindModel method. The return value of the BindModel method is returned to the result variable by the Get method.

Like the Do methods, the Get methods are generic methods. The one invoked in this example has this signature:

public TResult Get<T, TResult>(Func<T, TResult> function);

There are also overloads that works with the versions of the Func delegate that takes two, three and four parameters.

As the Do methods, the Get methods are convenience methods that let you concentrate on writing the test code you care about while it takes care of all the rest. You could have written a slightly more complex version that didn't use Get but instead used the CreateAnonymous method to create an Anonymous Variable for the ControllerContext, but this way is slightly more succinct.


Page 36 of 42

"Our team wholeheartedly endorses Mark. His expert service provides tremendous value."
Hire me!