Garth Kidd was so nice to point out to me that I hadn't needed stop where I did in my previous post, and he is, of course, correct. Taking a dependency on an Abstract Factory that doesn't take any contextual information (i.e. has no method parameters) is often an indication of a Leaky Abstraction. It indicates that the consumer has knowledge about the dependency's lifetime that it shouldn't have.

We can remove this flaw by introducing a Decorator of the IRepository<T> interface. Something like this should suffice:

public class FoundRepository<T> : IRepository<T>
{
    private readonly IRepository<T> repository;
 
    public FoundRepository(IRepositoryFinder<T> finder)
    {
        if (finder == null)
        {
            throw new ArgumentNullException("finder");
        }
 
        this.repository = finder.FindRepository();
    }
 
    /* Implement IRepository<T> by delegating to
     * this.repository */
}

This means that we can change the implementation of MyServiceOperation to this:

public void MyServiceOperation(
    IRepository<Customer> repository)
{
    // ...
}

This is much better, but this requires a couple of notes.

First of all we should keep in mind that since FoundRepository creates and saves an instance of IRepository right away, we should control the lifetime of FoundRepository. In essense, the lifetime should be tied to the specific service operation. Two concurrent invocations of MyServiceOperation should each receive separate instances of FoundRepository.

Many DI containers support Factory methods, so it may not even be necessary to implement FoundRepository explicitly. Rather, it would be possible to register IRepository<T> so that an instance is always created by invoking IRepositoryFinder<T>.FindRepository().


Comments

Mark, doesn't this contradict the rule to make constructors that get dependencies injected simple? You can't really know what happens inside finder.FindRepository() - it might even access the database...

I am asking, because I have a similar situation:
In the project I am currently refactoring, I created a lot of ConfiguredXYZ decorators that are modeled after this article.
They decorate classes, that have both dependencies and primitive types as ctor parameters. I use the decorator to read these primitive values from the configuration and to create an instance of the decorated class.

Example:


public ConfiguredCache(IConfigService configService, ISomeOtherDependency dep)
{
var expiryTime = configService.Get<DateTime>("ExpiryTimeForCache");
_configuredCache = new ExpiringCache(expiryTime, dep);
}

If IConfigService is implemented to read the database, I will hit the database in the constructor.

What do you think about it? Wouldn't it be better to create the instance of the ExpiringCache only when someone really needs it by invoking a method on ConfiguredCache?
2011-08-10 16:36 UTC
Agreed, but this is just one step in a refactoring process. What I would subsequently tend to do would be to refactor once more and let the Composition Root invoke the factory, which means that the consumer can now take only the result of invoking the factory as a single dependency.

You could also go the other way and perform a lazy evaluation of the dependency, but this is far more complicated to implement because it means that you'd be changing the state of the consumer. This again means that if you want to share the consumer as another dependency, you'll also need to think about thread safety.

In most cases I'd regard lazy evaluation as a premature optimization. As I have explained in another blog post, we shouldn't worry about the cost of composing an object graph.

In your example, IConfigService isn't really a dependency because the ConfiguredCache class doesn't depend on it - it only use it to get an instance of ExpiringCache. Change the constructor by removing the IConfigService and instead require an ExpiringCache (or the interface it implements). Some third party can take care of wiring up the IConfigService for you.

So what if it hits the database? It's probably going to do that sooner or later (meaning, typically, milliseconds or even seconds later) so does it matter? And will it happen often? A cache is only effective if it's shared, which means that we can share it with the Singleton lifetime. If we do that we only need to create the instance once per application, which again means that in the overall picture, that single database hit is insignificant.
2011-08-10 17:45 UTC
Thanks for your response.
Could you write a blog post about that refactoring step you would perform after you refactored to decorators?

I agree with your points in general, but they don't solve the problem that lead to those decorators in the first place:
The configuration of ExpiringCache is complex - not in the sample code, but in the real code. That was the whole reason for introducing those ConfiguredXYZ decorators, because I didn't want to have that complex configuration in the composition root. If you wouldn't create these decorators, how would you do it then? Create an ICache factory that returns a new ExpiringCache? This factory again would need IConfigService and ISomeOtherDependency to work, so from the constructor it would be the same as with the decorator...
And how would you register this factory with the container while reserving auto-wiring? Would you use Autofac's RegisterAdapter to register from ICacheFactory to ICache using the Create method of ICacheFactory?
2011-08-15 13:33 UTC
Why not perform the complex configuration in the Composition Root?
2011-08-15 13:39 UTC
Because I would like to keep it simple.
If I would perform this complex configuration in the Composition Root, it would become a monstrosity, because it would mean to put the other configurations in there, too.
I like my Composition Root "to just work", so I work with conventions. Putting the configurations in there would make this impossible...
2011-08-16 07:26 UTC
Why would it make it impossible?

The code that reads configuration etc. needs to go somewhere. The Composition Root is the correct place because it makes the rest of your application code decoupled from your configuration source(s).

There's nothing that prevents you from expressing conventions around configuration settings as well.
2011-08-16 07:39 UTC
I don't think I understand. If I would put this configuration code inside the composition root it would be several hundred lines long. Maybe even more. I seem to be missing something, because a composition root like this is a maintenance nightmare.
2011-08-16 09:25 UTC
Would the configuration code be longer if you put it in the Composition Root? If that's the case, I can't for the life of me imagine why this would be, so please explain.
2011-08-16 09:39 UTC
No, it wouldn't be longer if I would put it in the Composition Root. That would be very odd, I agree.
But: the configuration code that currently is spread out in several classes would all be in the composition root. Note: "the configuration code" really consists of several classes that are not related to each other. One class configures the SAP object (which SAP server to use, username, password etc.), another configures the cache (see example above), yet another configures the Logger (which file to write to) etc.
It might well be a problem with the architecture per se, because I came to what I have now from an architecture that was centered around a service locator. All the lower levels now are very good, they get dependencies injected and work with them, but I can't figure out, where to put this configuration code.
Because, in my opinion, this configuration code is "domain code". What I mean with this is that it requires special knowledge about the domain it is used in. For example, I don't think that the composition root should need to know about details of an SAP connection, just to configure the SapConnection class... This is more than just reading configuration values and passing them to a constructor. In the SAP example, it reads some values from the database and based on these values it makes some decisions and only after that, the object is created.
Having written it like this, it really sounds as if I should use Abstract Factories, but you objected to them in one of your earlier answers, which confuses me :-)
2011-08-16 10:05 UTC
The Composition Root doesn't have to be a single class. It's an architectural concept. It can have as many classes as you need as long as the stay together. Usually we implement the Composition Root in the same project as the application's entry point. This is where we wire everything together, so this particular place tends to not be (unit) testable. As such, we should go to great lengths to ensure that the entry point is a Humble Executable.

Thus it follows that the Composition Root can be as large as necessary, but it must contain no logic. My personal rule of thumb is that all members must have a cyclomatic complexity of 1.

Configuration isn't logic, but you may have application logic that depends on configuration values, so the trick is to separate the logic from the source of the values. The logic goes elsewhere, but the values are being dehydrated (or otherwise initialized) by the Composition Root.

I would consider a SAP connection an infrastructure concern. If you base your domain logic on knowledge of infrastructure specifics you're making life hard for yourself.
2011-08-16 10:40 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 Google Plus, or somewhere else with a permalink. Ping me with the link, and I may add it as a comment.

Published

Monday, 01 November 2010 21:19:06 UTC

Tags



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