ploeh blog danish software design

When creating resources with the ASP.NET Web API (beta) it's important to be able to create correct hyperlinks (you know, if it doesn't have hyperlinks, it's not REST). These hyperlinks may link to other resources in the same API, so it's important to keep the links consistent. A client following such a link should hit the desired resource.

This post describes an refactoring-safe approach to creating hyperlinks using the Web API RouteCollection and Expressions.

The Problem #

Obviously hyperlinks can be hard-coded, but since incoming requests are matched based on the Web API's RouteCollection, there's a risk that hard-coded links become disconnected from the API's incoming routes. In other words, hard-coding links is probably not a good idea.

For reference, the default route in the Web API looks like this:

routes.MapHttpRoute(
    name: "DefaultApi",
    routeTemplate: "{controller}/{id}",
    defaults: new
    {
        controller = "Catalog",
        id = RouteParameter.Optional
    }
);

A sample action fitting that route might look like this:

public Artist Get(string id)

where the Get method is defined by the ArtistController class.

Desired Outcome #

In order to provide a refactoring-safe way to create links to e.g. the artist resource, the strongly typed Resource Linker approach outlined by José F. Romaniello can be adopted. The IResourceLinker interface looks like this:

public interface IResourceLinker
{
    Uri GetUri<T>(Expression<Action<T>> method);
}

This makes it possible to create links like this:

var artist = new Artist
{
    Name = artistName,
    Links = new[]
    {
        new Link
        {
            Href = this.resourceLinker.GetUri<ArtistController>(r =>
                r.Get(artistsId)).ToString(),
            Rel = "self"
        }
    },
    // More crap goes here...
};

In this example, the resourceLinker field is an injected instance of IResourceLinker.

Since the input to the GetUri method is an Expression, it's being checked at compile time. It's refactoring-safe because a refactoring tool will be able to e.g. change the name of the method call in the Expression if the name of the method changes.

Example Implementation #

It's possible to implement IResourceLinker over a Web API RouteCollection. Here's an example implementation:

public class RouteLinker : IResourceLinker
{
    private Uri baseUri;
    private readonly HttpControllerContext ctx;
 
    public RouteLinker(Uri baseUri, HttpControllerContext ctx)
    {
        this.baseUri = baseUri;
        this.ctx = ctx;
    }
 
    public Uri GetUri<T>(Expression<Action<T>> method)
    {
        if (method == null)
            throw new ArgumentNullException("method");
 
        var methodCallExp = method.Body as MethodCallExpression;
        if (methodCallExp == null)
        {
            throw new ArgumentException("The expression's body must be a MethodCallExpression. The code block supplied should invoke a method.\nExample: x => x.Foo().", "method");
        }
 
        var routeValues = methodCallExp.Method.GetParameters()
            .ToDictionary(p => p.Name, p => GetValue(methodCallExp, p));
 
        var controllerName = methodCallExp.Method.ReflectedType.Name
            .ToLowerInvariant().Replace("controller", "");
        routeValues.Add("controller", controllerName);
 
        var relativeUri = this.ctx.Url.Route("DefaultApi", routeValues);
        return new Uri(this.baseUri, relativeUri);
    }
 
    private static object GetValue(MethodCallExpression methodCallExp,
        ParameterInfo p)
    {
        var arg = methodCallExp.Arguments[p.Position];
        var lambda = Expression.Lambda(arg);
        return lambda.Compile().DynamicInvoke().ToString();
    }
}

This isn't much different from José F. Romaniello's example, apart from the fact that it creates a dictionary of route values and then uses the UrlHelper.Route method to create a relative URI.

Please note that this is just an example implementation. For instance, the call to the Route method supplies the hard-coded string "DefaultApi" to indicate which route (from Global.asax) to use. I'll leave it as an exercise for the interested reader to provide a generalization of this implementation.

Comments

From time to time I encounter people who attempt to express an API in terms of IQueryable<T>. That's almost always a bad idea. In this post, I'll explain why.

In short, the IQueryable<T> interface is one of the best examples of a Header Interface that .NET has to offer. It's almost impossible to fully implement it.

Please note that this post is about the problematic aspects of designing an API around the IQueryable<T> interface. It's not an attack on the interface itself, which has its place in the BCL. It's also not an attack on all the wonderful LINQ methods available on IEnumerable<T>.

You can say that IQueryable<T> is one big Liskov Substitution Principle (LSP)violation just waiting to happen. In the next two section, I will apply Postel's law to explain why that is.

Consuming IQueryable<T> #

The first part of Postel's law applied to API design states that an API should be liberal in what it accepts. In other words, we are talking about input, so an API that consumes IQueryable<T> would take this generalized shape:

IFoo SomeMethod(IQueryable<Bar> q);

Is that a liberal requirement? It most certainly is not. Such an interface demands of any caller that they must be able to supply an implementation of IQueryable<Bar>. According to the LSP we must be able to supply any implementation without changing the correctness of the program. That goes for both the implementer of IQueryable<Bar> as well as the implementation of SomeMethod.

At this point it's important to keep in mind the purpose of IQueryable<T>: it's intended for implementation by query providers. In other words, this isn't just some sequence of Bar instances which can be filtered and projected; no, this is a query expression which is intended to be translated into a query somewhere else - most often some dialect of SQL.

That's quite a demand to put on the caller.

It's certainly a powerful interface (or so it would seem), but is it really necessary? Does SomeMethod really need to be able to perform arbitrarily complex queries against a data source?

In one recent discussion, it turns out that all the developer really wanted to do was to be able to select based on a handful of simple criteria. In another case, the developer only wanted to do simple paging.

Such requirements could be modeled much simpler without making huge demands on the caller. In both cases, we could provide specialized Query Objects instead, or perhaps even simpler just a set of specialized queries:

IFoo FindById(int fooId);
 
IFoo FindByCorrelationId(int correlationId);

Or, in the case of paging:

IEnumerable<IFoo> GetFoos(int page);

This is certainly much more liberal in that it requires the caller to supply only the required information in order to implement the methods. Designing APIs in terms of Role Interfaces instead of Header Interfaces makes the APIs much more flexible. This will enable you to respond to change.

Exposing IQueryable<T> #

The other part of Postel's law states that an API should be conservative in what it sends. In other words, a method must guarantee that the data it returns conforms rigorously to the contract between caller and implementer.

A method returning IQueryable<T> would take this generalized shape:

IQueryable<Bar> GetBars();

When designing APIs, a huge part of the contract is defined by the interface (or base class). Thus, the return type of a method specifies a conservative guarantee about the returned data. In the case of returning IQueryable<Bar> the method thus guarantees that it will return a complete implementation of IQueryable<Bar>.

Is that conservative?

Once again invoking the LSP, a consumer must be able to do anything allowed by IQueryable<Bar> without changing the correctness of the program.

That's a big honking promise to make.

Who can keep that promise?

Current Implementations #

Implementing IQueryable<T> is a huge undertaking. If you don't believe me, just take a look at the official Building an IQueryable provider series of blog posts. Even so, the interface is so flexible and expressive that with a single exception, it's always possible to write a query that a given provider can't translate.

Have you ever worked with LINQ to Entities or another ORM and received a NotSupportedException? Lots of people have. In fact, with a single exception, it's my firm belief that all existing implementations violate the LSP (in fact, I challenge my readers to refer me to a real, publicly available implementation of IQueryable<T> that can accept any expression I throw at it, and I'll ship a free copy of my book to the first reader to do so).

Furthermore, the subset of features that each implementation supports varies from query provider to query provider. An expression that can be translated by the Entity framework may not work with Microsoft's OData query provider.

The only implementation that fully implements IQueryable<T> is the in-memory implementation (and referring to this one does not earn you a free book). Ironically, this implementation can be considered a Null Object implementation and goes against the whole purpose of the IQueryable<T> interface exactly because it doesn't translate the expression to another language.

Why This Matters #

You may think this is all a theoretical exercise, but it actually does matter. When writing Clean Code, it's important to design an API in such a way that it's clear what it does.

An interface like this makes false guarantees:

public interface IRepository
{
    IQueryable<T> Query<T>();
}

According to the LSP and Postel's law, it would seem to guarantee that you can write any query expression (no matter how complex) against the returned instance, and it would always work.

In practice, this is never going to happen.

Programmers who define such interfaces invariably have a specific ORM in mind, and they implicitly tend to stay within the bounds they know are safe for that specific ORM. This is a leaky abstraction.

If you have a specific ORM in mind, then be explicit about it. Don't hide it behind an interface. It creates the illusion that you can replace one implementation with another. In practice, that's impossible. Imagine attempting to provide an implementation over an Event Store.

The cake is a lie.

Comments

Note 2014-04-03 19:46 UTC: This post describes how to address various Dependency Injection issues with a Community Technical Preview (CTP) of ASP.NET Web API 1. Unless you're still using that 2012 CTP, it's no longer relevant. Instead, refer to my article about Dependency Injection with the final version of Web API 1 (which is also relevant for Web API 2).

Like the WCF Web API, the new ASP.NET Web API supports Dependency Injection (DI), but the approach is different and the resulting code you'll have to write is likely to be more complex. This post describes how to enable robust DI with the new Web API. Since this is based on the beta release, I hope that it will become easier in the final release.

At first glance, enabling DI on an ASP.NET Web API looks seductively simple. As always, though, the devil is in the details. Nikos Baxevanis has already provided a more thorough description, but it's even more tricky than that.

Protocol #

To enable DI, all you have to do is to call the SetResolver method, right? It even has an overload that enables you to supply two code blocks instead of implementing an interface (although you can certainly also implement IDependencyResolver). Could it be any easier than that?

Yes, it most certainly could.

Imagine that you'd like to hook up your DI Container of choice. As a first attempt, you try something like this:

GlobalConfiguration.Configuration.ServiceResolver.SetResolver(
    t => this.container.Resolve(t),
    t => this.container.ResolveAll(t).Cast<object>());

This compiles. Does it work? Yes, but in a rather scary manner. Although it satisfies the interface, it doesn't satisfy the protocol ("an interface describes whether two components will fit together, while a protocol describes whether they will work together." (GOOS, p. 58)).

The protocol, in this case, is that if you (or rather the container) can't resolve the type, you should return null. What's even worse is that if your code throws an exception (any exception, apparently), DependencyResolver will suppress it. In case you didn't know, this is strongly frowned upon in the .NET Framework Design Guidelines.

Even so, the official introduction article instead chooses to play along with the protocol and explicitly handle any exceptions. Something along the lines of this ugly code:

GlobalConfiguration.Configuration.ServiceResolver.SetResolver(
    t =>
    {
        try
        {
            return this.container.Resolve(t);
        }
        catch (ComponentNotFoundException)
        {
            return null;
        }
    },
    t =>
    {
        try
        {
            return this.container.ResolveAll(t).Cast<object>();
        }
        catch (ComponentNotFoundException)
        {
            return new List<object>();
        }
    }
);

Notice how try/catch is used for control flow - another major no no in the .NET Framework Design Guidelines.

At least with a good DI Container, we can do something like this instead:

GlobalConfiguration.Configuration.ServiceResolver.SetResolver(
    t => this.container.Kernel.HasComponent(t) ?
        this.container.Resolve(t) :
        null,
    t => this.container.ResolveAll(t).Cast<object>());

Still, first impressions don't exactly inspire trust in the implementation...

API Design Issues #

Next, I would like to direct your attention to the DependencyResolver API. At its core, it looks like this:

public interface IDependencyResolver
{
    object GetService(Type serviceType);
    IEnumerable<object> GetServices(Type serviceType);
}

It can create objects, but what about decommissioning? What if, deep in a dependency graph, a Controller contains an IDisposable object? This is not a particularly exotic scenario - it might be an instance of an Entity Framework ObjectContext. While an ApiController itself implements IDisposable, it may not know that it contains an injected object graph with one or more IDisposable leaf nodes.

It's a fundamental rule of DI that you must Release what you Resolve. That's not possible with the DependencyResolver API. The result may be memory leaks.

Fortunately, it turns out that there's a fix for this (at least for Controllers). Unfortunately, this workaround leverages another design problem with DependencyResolver.

Mixed Responsibilities #

It turns out that when you wire a custom resolver up with the SetResolver method, the ASP.NET Web API will query your custom resolver (such as a DI Container) for not only your application classes, but also for its own infrastructure components. That surprised me a bit because of the mixed responsibility, but at least this is a useful hook.

One of the first types the framework will ask for is an instance of IHttpControllerFactory, which looks like this:

public interface IHttpControllerFactory
{
    IHttpController CreateController(HttpControllerContext controllerContext,
        string controllerName);
    void ReleaseController(IHttpController controller);
}

Fortunately, this interface has a Release hook, so at least it's possible to release Controller instances, which is most important because there will be a lot of them (one per HTTP request).

Discoverability Issues #

The IHttpControllerFactory looks a lot like the well-known ASP.NET MVC IControllerFactory interface, but there are subtle differences. In ASP.NET MVC, there's a DefaultControllerFactory with appropriate virtual methods one can overwrite (it follows the Template Method pattern).

There's also a DefaultControllerFactory in the Web API, but unfortunately no Template Methods to override. While I could write an algorithm that maps from the controllerName parameter to a type which can be passed to a DI Container, I'd rather prefer to be able to reuse the implementation which the DefaultControllerFactory contains.

In ASP.NET MVC, this is possible by overriding the GetControllerInstance method, but it turns out that the Web API (beta) does this slightly differently. It favors composition over inheritance (which is actually a good thing, so kudos for that), so after mapping controllerName to a Type instance, it invokes an instance of the IHttpControllerActivator interface (did I hear anyone say "FactoryFactory?"). Very loosely coupled (good), but not very discoverable (not so good). It would have been more discoverable if DefaultControllerFactory had used Constructor Injection to get its dependency, rather than relying on the Service Locator which DependencyResolver really is.

However, this is only an issue if you need to hook into the Controller creation process, e.g. in order to capture the HttpControllerContext for further use. In normal scenarios, despite what Nikos Baxevanis describes in his blog post, you don't have to override or implement IHttpControllerFactory.CreateController. The DependencyResolver infrastructure will automatically invoke your GetService implementation (or the corresponding code block) whenever a Controller instance is required.

Releasing Controllers #

The easiest way to make sure that all Controller instances are being properly released is to derive a class from DefaultControllerFactory and override the ReleaseController method:

public class ReleasingControllerFactory : DefaultHttpControllerFactory
{
    private readonly Action<object> release;
 
    public ReleasingControllerFactory(Action<object> releaseCallback,
        HttpConfiguration configuration)
        : base(configuration)
    {
        this.release = releaseCallback;
    }
 
    public override void ReleaseController(IHttpController controller)
    {
        this.release(controller);
        base.ReleaseController(controller);
    }
}

Notice that it's not necessary to override the CreateController method, since the default implementation is good enough - it'll ask the DependencyResolver for an instance of IHttpControllerActivator, which will again ask the DependencyResolver for an instance of the Controller type, in the end invoking your custom GetObject implemention.

To keep the above example generic, I just injected an Action<object> into ReleasingControllerFactory - I really don't wish to turn this into a discussion about the merits and demerits of various DI Containers. In any case, I'll leave it as an exercise to you to wire up your favorite DI Container so that the releaseCallback is actually a call to the container's Release method.

Lifetime Cycles of Infrastructure Components #

Before I conclude, I'd like to point out another POLA violation that hit me during my investigation.

The ASP.NET Web API utilizes DependencyResolver to resolve its own infrastructure types (such as IHttpControllerFactory, IHttpControllerActivator, etc.). Any custom DependencyResolver you supply will also be queried for these types. However:

When resolving infrastructure components, the Web API doesn't respect any custom lifecycle you may have defined for these components.

At a certain point while I investigated this, I wanted to configure a custom IHttpControllerActivator to have a Web Request Context (my book, section 8.3.4) - in other words, I wanted to create a new instance of IHttpControllerActivator for each incoming HTTP request.

This is not possible. The framework queries a custom DependencyResolver for an infrastructure type, but even when it receives an instance (i.e. not null), it doesn't trust the DependencyResolver to efficiently manage the lifetime of that instance. Instead, it caches this instance for ever, and never asks for it again. This is, in my opinion, a mistaken responsibility, and I hope it will be corrected in the final release.

Concluding Thoughts #

Wiring up the ASP.NET Web API with robust DI is possible, but much harder than it ought to be. Suggestions for improvements are:

• A Release hook in DependencyResolver.
• The framework itself should trust the DependencyResolver to efficiently manage lifetime of all objects it create.

As I've described, there are other places were minor adjustments would be helpful, but these two suggestions are the most important ones.

Update (2012.03.21): I've posted this feedback to the product group on uservoice and Connect - if you agree, please visit those sites and vote up the issues.

Comments

Now that the WCF Web API has ‘become' the ASP.NET Web API, I've had to migrate a semi-complex code base from the old to the new framework. These are my notes from that process.

Migrating Project References #

As far as I can tell, the ASP.NET Web API isn't just a port of the WCF Web API. At a cursory glance, it looks like a complete reimplementation. If it's a port of the old code, it's at least a rather radical one. The assemblies have completely different names, and so on.

Both old and new project, however, are based on NuGet packages, so it wasn't particularly hard to change.

To remove the old project references, I ran this NuGet command:

Uninstall-Package webapi.all -RemoveDependencies

followed by

Install-Package aspnetwebapi

to install the project references for the ASP.NET Web API.

Rename Resources to Controllers #

In the WCF Web API, there was no naming convention for the various resource classes. In the quickstarts, they were sometimes called Apis (like ContactsApi), and I called mine Resources (like CatalogResource). Whatever your naming convention was, the easiest things is to find them all and rename them to end with Controller (e.g. CatalogController).

AFAICT you can change the naming convention, but I didn't care enough to do so.

Derive Controllers from ApiController #

Unless you care to manually implement IHttpController, each Controller should derive from ApiController:

public class CatalogController : ApiController

Remove Attributes #

The WCF Web API uses the [WebGet] and [WebInvoke] attributes. The ASP.NET Web API, on the other hand, uses routes, so I removed all the attributes, including their UriTemplates:

//[WebGet(UriTemplate = "/")]
public Catalog GetRoot()

Add Routes #

As a replacement for attributes and UriTemplates, I added HTTP routes:

routes.MapHttpRoute(
    name: "DefaultApi",
    routeTemplate: "{controller}/{id}",
    defaults: new { controller = "Catalog", id = RouteParameter.Optional }
);

The MapHttpRoute method is an extension method defined in the System.Web.Http namespace, so I had to add a using directive for it.

Composition #

Wiring up Controllers with Constructor Injection turned out to be rather painful. For a starting point, I used Nikos Baxevanis' guide, but it turns out there are further subtleties which should be addressed (more about this later, but to prevent a stream of comments about the DependencyResolver API: yes, I know about that, but it's inadequate for a number of reasons).

Media Types #

In the ASP.NET Web API application/json is now the default media type format if the client doesn't supply any Accept header. For the WCF Web API I had to resort to a hack to change the default, so this was a pleasant surprise.

It's still pretty easy to add more supported media types:

GlobalConfiguration.Configuration.Formatters.XmlFormatter
    .SupportedMediaTypes.Add(
        new MediaTypeHeaderValue("application/vnd.247e.artist+xml"));
GlobalConfiguration.Configuration.Formatters.JsonFormatter
    .SupportedMediaTypes.Add(
        new MediaTypeHeaderValue("application/vnd.247e.artist+json"));

(Talk about a Law of Demeter violation, BTW...)

However, due to an over-reliance on global state, it's not so easy to figure out how one would go about mapping certain media types to only a single Controller. This was much easier in the WCF Web API because it was possible to assign a separate configuration instance to each Controller/Api/Resource/Service/Whatever... This, I've still to figure out how to do...

Comments

Abstract Factory is a tremendously useful pattern when used with Dependency Injection (DI). While I've repeatedly described how it can be used to solve various problems in DI, apparently I've never described how to implement one. As a comment to an older blog post of mine, Thomas Jaskula asks how I'd implement the IOrderShipperFactory.

To stay consistent with the old order shipper scenario, this blog post outlines three alternative ways to implement the IOrderShipperFactory interface.

To make it a bit more challenging, the implementation should create instances of the OrderShipper2 class, which itself has a dependency:

public class OrderShipper2 : IOrderShipper
{
    private readonly IChannel channel;
 
    public OrderShipper2(IChannel channel)
    {
        if (channel == null)
            throw new ArgumentNullException("channel");
 
        this.channel = channel;
    }
 
    public void Ship(Order order)
    {
        // Ship the order and
        // raise a domain event over this.channel
    }
}

In order to be able to create an instance of OrderShipper2, any factory implementation must be able to supply an IChannel instance.

Manually Coded Factory #

The first option is to manually wire up the OrderShipper2 instance within the factory:

public class ManualOrderShipperFactory :
    IOrderShipperFactory
{
    private readonly IChannel channel;
 
    public ManualOrderShipperFactory(IChannel channel)
    {
        if (channel == null)
            throw new ArgumentNullException("channel");
 
        this.channel = channel;
    }
 
    public IOrderShipper Create()
    {
        return new OrderShipper2(this.channel);
    }
}

This has the advantage that it's easy to understand. It can be unit tested and implemented in the same library that also contains OrderShipper2 itself. This means that any client of that library is supplied with a read-to-use implementation.

The disadvantage of this approach is that if/when the constructor of OrderShipper2 changes, the ManualOrderShipperFactory class must also be corrected. Pragmatically, this may not be a big deal, but one could successfully argue that this violates the Open/Closed Principle.

Container-based Factory #

Another option is to make the implementation a thin Adapter over a DI Container - in this example Castle Windsor:

public class ContainerFactory : IOrderShipperFactory
{
    private IWindsorContainer container;
 
    public ContainerFactory(IWindsorContainer container)
    {
        if (container == null)
            throw new ArgumentNullException("container");
 
        this.container = container;
    }
 
    public IOrderShipper Create()
    {
        return this.container.Resolve<IOrderShipper>();
    }
}

But wait! Isn't this an application of the Service Locator anti-pattern? Not if this class is part of the Composition Root.

If this implementation was placed in the same library as OrderShipper2 itself, it would mean that the library would have a hard dependency on the container. In such a case, it would certainly be a Service Locator.

However, when a Composition Root already references a container, it makes sense to place the ContainerFactory class there. This changes its role to the pure infrastructure component it really ought to be. This seems more SOLID, but the disadvantage is that there's no longer a ready-to-use implementation packaged together with the LazyOrderShipper2 class. All new clients must supply their own implementation.

Dynamic Proxy #

The third option is to basically reduce the principle behind the container-based factory to its core. Why bother writing even a thin Adapter if one can be automatically generated.

With Castle Windsor, the Typed Factory Facility makes this possible:

container.AddFacility<TypedFactoryFacility>();
container.Register(Component
    .For<IOrderShipperFactory>()
    .AsFactory());
var factory =
    container.Resolve<IOrderShipperFactory>();

There is no longer any code which implements IOrderShipperFactory. Instead, a class conceptually similar to the ContainerFactory class above is dynamically generated and emitted at runtime.

While the code never materializes, conceptually, such a dynamically emitted implementation is still part of the Composition Root.

This approach has the advantage that it's very DRY, but the disadvantages are similar to the container-based implementation above: there's no longer a ready-to-use implementation. There's also the additional disadvantage that out of the three alternative here outlined, the proxy-based implementation is the most difficult to understand.

Comments

For years, layered application architecture has been a de-facto standard for loosely coupled architectures, but the question is: does the layering really provide benefit?

In theory, layering is a way to decouple concerns so that UI concerns or data access technologies don't pollute the domain model. However, this style of architecture seems to come at a pretty steep price: there's a lot of mapping going on between the layers. Is it really worth the price, or is it OK to define data structures that cut across the various layers?

The short answer is that if you cut across the layers, it's no longer a layered application. However, the price of layering may still be too high.

In this post I'll examine the problem and the proposed solution and demonstrate why none of them are particularly good. In the end, I'll provide a pointer going forward.

Proper Layering #

To understand the problem with layering, I'll describe a fairly simple example. Assume that you are building a music service and you've been asked to render a top 10 list for the day. It would have to look something like this in a web page:

As part of rendering the list, you must color the Movement values accordingly using CSS.

A properly layered architecture would look something like this:

Each layer defines some services and some data-carrying classes (Entities, if you want to stick with the Domain-Driven Design terminology). The Track class is defined by the Domain layer, while the TopTrackViewModel class is defined in the User Interface layer, and so on. If you are wondering about why Track is used to communicate both up and down, this is because the Domain layer should be the most decoupled layer, so the other layers exist to serve it. In fact, this is just a vertical representation of the Ports and Adapters architecture, with the Domain Model sitting in the center.

This architecture is very decoupled, but comes at the cost of much mapping and seemingly redundant repetition. To demonstrate why that is, I'm going to show you some of the code. This is an ASP.NET MVC application, so the Controller is an obvious place to start:

public ViewResult Index()
{
    var date = DateTime.Now.Date;
    var topTracks = this.trackService.GetTopFor(date);
    return this.View(topTracks);
}

This doesn't look so bad. It asks an ITrackService for the top tracks for the day and returns a list of TopTrackViewModel instances. This is the implementation of the track service:

public IEnumerable<TopTrackViewModel> GetTopFor(
    DateTime date)
{
    var today = DateTime.Now.Date;
    var yesterDay = today - TimeSpan.FromDays(1);
 
    var todaysTracks = 
        this.repository.GetTopFor(today).ToList();
    var yesterdaysTracks = 
        this.repository.GetTopFor(yesterDay).ToList();
 
    var length = todaysTracks.Count;
    var positions = Enumerable.Range(1, length);
 
    return from tt in todaysTracks.Zip(
                positions, (t, p) =>
                    new { Position = p, Track = t })
            let yp = (from yt in yesterdaysTracks.Zip(
                            positions, (t, p) => 
                                new
                                {
                                    Position = p,
                                    Track = t
                                })
                        where yt.Track.Id == tt.Track.Id
                        select yt.Position)
                        .DefaultIfEmpty(-1)
                        .Single()
            let cssClass = GetCssClass(tt.Position, yp)
            select new TopTrackViewModel
            {
                Position = tt.Position,
                Name = tt.Track.Name,
                Artist = tt.Track.Artist,
                CssClass = cssClass
            };
}
 
private static string GetCssClass(
    int todaysPosition, int yesterdaysPosition)
{
    if (yesterdaysPosition < 0)
        return "new";
    if (todaysPosition < yesterdaysPosition)
        return "up";
    if (todaysPosition == yesterdaysPosition)
        return "same";
    return "down";
}

While that looks fairly complex, there's really not a lot of mapping going on. Most of the work is spent getting the top 10 track for today and yesterday. For each position on today's top 10, the query finds the position of the same track on yesterday's top 10 and creates a TopTrackViewModel instance accordingly.

Here's the only mapping code involved:

select new TopTrackViewModel
{
    Position = tt.Position,
    Name = tt.Track.Name,
    Artist = tt.Track.Artist,
    CssClass = cssClass
};

This maps from a Track (a Domain class) to a TopTrackViewModel (a UI class).

This is the relevant implementation of the repository:

public IEnumerable<Track> GetTopFor(DateTime date)
{
    var dbTracks = this.GetTopTracks(date);
    foreach (var dbTrack in dbTracks)
    {
        yield return new Track(
            dbTrack.Id,
            dbTrack.Name,
            dbTrack.Artist);
    }
}

You may be wondering about the translation from DbTrack to Track. In this case you can assume that the DbTrack class is a class representation of a database table, modeled along the lines of your favorite ORM. The Track class, on the other hand, is a proper object-oriented class which protects its invariants:

public class Track
{
    private readonly int id;
    private string name;
    private string artist;
 
    public Track(int id, string name, string artist)
    {
        if (name == null)
            throw new ArgumentNullException("name");
        if (artist == null)
            throw new ArgumentNullException("artist");
 
        this.id = id;
        this.name = name;
        this.artist = artist;
    }
 
    public int Id
    {
        get { return this.id; }
    }
 
    public string Name
    {
        get { return this.name; }
        set
        {
            if (value == null)
                throw new ArgumentNullException("value");
 
            this.name = value;
        }
    }
 
    public string Artist
    {
        get { return this.artist; }
        set
        {
            if (value == null)
                throw new ArgumentNullException("value");
 
            this.artist = value;
        }
    }
}

No ORM I've encountered so far has been able to properly address such invariants - particularly the non-default constructor seems to be a showstopper. This is the reason a separate DbTrack class is required, even for ORMs with so-called POCO support.

In summary, that's a lot of mapping. What would be involved if a new field is required in the top 10 table? Imagine that you are being asked to provide the release label as an extra column.

1. A Label column must be added to the database schema and the DbTrack class.
2. A Label property must be added to the Track class.
3. The mapping from DbTrack to Track must be updated.
4. A Label property must be added to the TopTrackViewModel class.
5. The mapping from Track to TopTrackViewModel must be updated.
6. The UI must be updated.

That's a lot of work in order to add a single data element, and this is even a read-only scenario! Is it really worth it?

Cross-Cutting Entities #

Is strict separation between layers really so important? What would happen if Entities were allowed to travel across all layers? Would that really be so bad?

Such an architecture is often drawn like this:

Now, a single Track class is allowed to travel from layer to layer in order to avoid mapping. The controller code hasn't really changed, although the model returned to the View is no longer a sequence of TopTrackViewModel, but simply a sequence of Track instances:

public ViewResult Index()
{
    var date = DateTime.Now.Date;
    var topTracks = this.trackService.GetTopFor(date);
    return this.View(topTracks);}

The GetTopFor method also looks familiar:

public IEnumerable<Track> GetTopFor(DateTime date)
{
    var today = DateTime.Now.Date;
    var yesterDay = today - TimeSpan.FromDays(1);
 
    var todaysTracks = 
        this.repository.GetTopFor(today).ToList();
    var yesterdaysTracks = 
        this.repository.GetTopFor(yesterDay).ToList();
 
    var length = todaysTracks.Count;
    var positions = Enumerable.Range(1, length);
 
    return from tt in todaysTracks.Zip(
                positions, (t, p) => 
                    new { Position = p, Track = t })
            let yp = (from yt in yesterdaysTracks.Zip(
                            positions, (t, p) =>
                                new
                                {
                                    Position = p,
                                    Track = t
                                })
                        where yt.Track.Id == tt.Track.Id
                        select yt.Position)
                        .DefaultIfEmpty(-1)
                        .Single()
            let cssClass = GetCssClass(tt.Position, yp)
            select Enrich(
                tt.Track, tt.Position, cssClass);
}
 
private static string GetCssClass(
    int todaysPosition, int yesterdaysPosition)
{
    if (yesterdaysPosition < 0)
        return "new";
    if (todaysPosition < yesterdaysPosition)
        return "up";
    if (todaysPosition == yesterdaysPosition)
        return "same";
    return "down";
}
 
private static Track Enrich(
    Track track, int position, string cssClass)
{
    track.Position = position;
    track.CssClass = cssClass;
    return track;
}

Whether or not much has been gained is likely to be a subjective assessment. While mapping is no longer taking place, it's still necessary to assign a CSS Class and Position to the track before handing it off to the View. This is the responsibility of the new Enrich method:

private static Track Enrich(
    Track track, int position, string cssClass)
{
    track.Position = position;
    track.CssClass = cssClass;
    return track;
}

If not much is gained at the UI layer, perhaps the data access layer has become simpler? This is, indeed, the case:

public IEnumerable<Track> GetTopFor(DateTime date)
{
    return this.GetTopTracks(date);
}

If, hypothetically, you were asked to add a label to the top 10 table it would be much simpler:

1. A Label column must be added to the database schema and the Track class.
2. The UI must be updated.

This looks good. Are there any disadvantages? Yes, certainly. Consider the Track class:

public class Track
{
    public int Id { get; set; }
 
    public string Name { get; set; }
 
    public string Artist { get; set; }
 
    public int Position { get; set; }
 
    public string CssClass { get; set; }
}

It also looks simpler than before, but this is actually not particularly beneficial, as it doesn't protect its invariants. In order to play nice with the ORM of your choice, it must have a default constructor. It also has automatic properties. However, most insidiously, it also somehow gained the Position and CssClass properties.

What does the Position property imply outside of the context of a top 10 list? A position in relation to what?

Even worse, why do we have a property called CssClass? CSS is a very web-specific technology so why is this property available to the Data Access and Domain layers? How does this fit if you are ever asked to build a native desktop app based on the Domain Model? Or a REST API? Or a batch job?

When Entities are allowed to travel along layers, the layers basically collapse. UI concerns and data access concerns will inevitably be mixed up. You may think you have layers, but you don't.

Is that such a bad thing, though?

Perhaps not, but I think it's worth pointing out:

The choice is whether or not you want to build a layered application. If you want layering, the separation must be strict. If it isn't, it's not a layered application.

There may be great benefits to be gained from allowing Entities to travel from database to user interface. Much mapping cost goes away, but you must realize that then you're no longer building a layered application - now you're building a web application (or whichever other type of app you're initially building).

Further Thoughts #

It's a common question: how hard is it to add a new field to the user interface?

The underlying assumption is that the field must somehow originate from a corresponding database column. If this is the case, mapping seems to be in the way.

However, if this is the major concern about the application you're currently building, it's a strong indication that you are building a CRUD application. If that's the case, you probably don't need a Domain Model at all. Go ahead and let your ORM POCO classes travel up and down the stack, but don't bother creating layers: you'll be building a monolithic application no matter how hard you try not to.

In the end it looks as though none of the options outlined in this article are particularly good. Strict layering leads to too much mapping, and no mapping leads to monolithic applications. Personally, I've certainly written quite a lot of strictly layered applications, and while the separation of concerns was good, I was never happy with the mapping overhead.

At the heart of the problem is the CRUDy nature of many applications. In such applications, complex object graphs are traveling up and down the stack. The trick is to avoid complex object graphs.

Move less data around and things are likely to become simpler. This is one of the many interesting promises of CQRS, and particularly it's asymmetric approach to data.

To be honest, I wish I had fully realized this when I started writing my book, but when I finally realized what I'd implicitly felt for long, it was too late to change direction. Rest assured that nothing in the book is fundamentally flawed. The patterns etc. still hold, but the examples could have been cleaner if the sample applications had taken a CQRS-like direction instead of strict layering.

Comments

A common criticism of loosely coupled code is that it's harder to understand. How do you see the big picture of an application when loose coupling is everywhere? When the entire code base has been programmed against interfaces instead of concrete classes, how do we understand how the objects are wired and how they interact?

In this post, I'll provide answers on various levels, from high-level architecture over object-oriented principles to more nitty-gritty code. Before I do that, however, I'd like to pose a set of questions you should always be prepared to answer.

Mu #

My first reaction to that sort of question is: you say loosely coupled code is harder to understand. Harder than what?

If we are talking about a non-trivial application, odds are that it's going to take some time to understand the code base - whether or not it's loosely coupled. Agreed: understanding a loosely coupled code base takes some work, but so does understanding a tightly coupled code base. The question is whether it's harder to understand a loosely coupled code base?

Imagine that I'm having a discussion about this subject with Mary Rowan from my book.

Mary: “Loosely coupled code is harder to understand.”

Me: “Why do you think that is?”

Mary: “It's very hard to navigate the code base because I always end up at an interface.”

Me: “Why is that a problem?”

Mary: “Because I don't know what the interface does.”

At this point I'm very tempted to answer Mu. An interfaces doesn't do anything - that's the whole point of it. According to the Liskov Substitution Principle (LSP), a consumer shouldn't have to care about what happens on the other side of the interface.

However, developers used to tightly coupled code aren't used to think about services in this way. They are used to navigate the code base from consumer to service to understand how the two of them interact, and I will gladly admit this: in principle, that's impossible to do in a loosely coupled code base. I'll return to this subject in a little while, but first I want to discuss some strategies for understanding a loosely coupled code base.

Architecture and Documentation #

Yes: documentation. Don't dismiss it. While I agree with Uncle Bob and like-minded spirits that the code is the documentation, a two-page document that outlines the Big Picture might save you from many hours of code archeology.

The typical agile mindset is to minimize documentation because it tends to lose touch with the code base, but even so, it should be possible to maintain a two-page high-level document so that it stays up to date. Consider the alternative: if you have so much architectural churn that even a two-page overview regularly falls behind, then you're probably having a greater problem than understanding your loosely coupled code base.

Maintaining such a document isn't adverse to the agile spirit. You'll find the same advice in Lean Architecture (p. 127). Don't underestimate the value of such a document.

See the Forest Instead of the Trees #

Understanding a loosely coupled code base typically tends to require a shift of mindset.

Recall my discussion with Mary Rowan. The criticism of loose coupling is that it's difficult to understand which collaborators are being invoked. A developer like Mary Rowan is used to learn a code base by understanding all the myriad concrete details of it. In effect, while there may be ‘classes' around, there are no abstractions in place. In order to understand the behavior of a user interface element, it's necessary to also understand what's happening in the database - and everything in between.

A loosely coupled code base shouldn't be like that.

The entire purpose of loose coupling is that we should be able to reason about a part (a ‘unit', if you will) without understanding the whole.

In a tightly coupled code base, it's often impossible to see the forest for the trees. Although we developers are good at relating to details, a tightly coupled code base requires us to be able to contain the entire code base in our heads in order to understand it. As the size of the code base grows, this becomes increasingly difficult.

In a loosely coupled code base, on the other hand, it should be possible to understand smaller parts in isolation. However, I purposely wrote “should be”, because that's not always the case. Often, a so-called “loosely coupled” code base violates basic principles of object-oriented design.

RAP #

The criticism that it's hard to see “what's on the other side of an interface” is, in my opinion, central. It betrays a mindset which is still tightly coupled.

In many code bases there's often a single implementation of a given interface, so developers can be forgiven if they think about an interface as only a piece of friction that prevents them from reaching the concrete class on the other side. However, if that's the case with most of the interfaces in a code base, it signifies a violation of the Reused Abstractions Principle (RAP) more than it signifies loose coupling.

Jim Cooper, a reader of my book, put it quite eloquently on the book's forum:

“So many people think that using an interface magically decouples their code. It doesn't. It only changes the nature of the coupling. If you don't believe that, try changing a method signature in an interface - none of the code containing method references or any of the implementing classes will compile. I call that coupled”

Refactoring tools aside, I completely agree with this statement. The RAP is a test we can use to verify whether or not an interface is truly reusable - what better test is there than to actually reuse your interfaces?

The corollary of this discussion is that if a code base is massively violating the RAP then it's going to be hard to understand. It has all the disadvantages of loose coupling with few of the benefits. If that's the case, you would gain more benefit from making it either more tightly coupled or truly loosely coupled.

What does “truly loosely coupled” mean?

LSP #

According to the LSP a consumer must not concern itself with “what's on the other side of the interface”. It should be possible to replace any implementation with any other implementation of the same interface without changing the correctness of the program.

This is why I previously said that in a truly loosely coupled code base, it isn't ‘hard' to understand “what's on the other side of the interface” - it's impossible. At design-time, there's nothing ‘behind' the interface. The interface is what you are programming against. It's all there is.

Mary has been listening to all of this, and now she protests:

Mary: “At run-time, there's going to be a concrete class behind the interface.”

Me (being annoyingly pedantic): “Not quite. There's going to be an instance of a concrete class which the consumer invokes via the interface it implements.”

Mary: “Yes, but I still need to know which concrete class is going to be invoked.”

Me: “Why?”

Mary: “Because otherwise I don't know what's going to happen when I invoke the method.”

This type of answer often betrays a much more fundamental problem in a code base.

CQS #

Now we are getting into the nitty-gritty details of class design. What would you expect that the following method does?

public List<Order> GetOpenOrders(Customer customer)

The method name indicates that it gets open orders, and the signature seems to back it up. A single database query might be involved, since this looks a lot like a read-operation. A quick glance at the implementation seems to verify that first impression:

public List<Order> GetOpenOrders(Customer customer)
{
    var orders = GetOrders(customer);
    return (from o in orders
            where o.Status == OrderStatus.Open
            select o).ToList();
}

Is it safe to assume that this is a side-effect-free method call? As it turns out, this is far from the case in this particular code base:

private List<Order> GetOrders(Customer customer)
{
    var gw = new CustomerGateway(this.ConnectionString);
    var orders = gw.GetOrders(customer);
    AuditOrders(orders);
    FixCustomer(gw, orders, customer);
    return orders;
}

The devil is in the details. What does AuditOrders do? And what does FixCustomer do? One method at a time:

private void AuditOrders(List<Order> orders)
{
    var user = Thread.CurrentPrincipal.Identity.ToString();
    var gw = new OrderGateway(this.ConnectionString);
    foreach (var o in orders)
    {
        var clone = o.Clone();
        var ar = new AuditRecord
        {
            Time = DateTime.Now,
            User = user
        };
        clone.AuditTrail.Add(ar);
        gw.Update(clone);
 
        // We don't want the consumer to see the audit trail.
        o.AuditTrail.Clear();
    }
}

OK, it turns out that this method actually makes a copy of each and every order and updates that copy, writing it back to the database in order to leave behind an audit trail. It also mutates each order before returning to the caller. Not only does this method result in an unexpected N+1 problem, it also mutates its input, and perhaps even more surprising, it's leaving the system in a state where the in-memory object is different from the database. This could lead to some pretty interesting bugs.

Then what happens in the FixCustomer method? This:

// Fixes the customer status field if there were orders
// added directly through the database.
private static void FixCustomer(CustomerGateway gw,
    List<Order> orders, Customer customer)
{
    var total = orders.Sum(o => o.Total);
    if (customer.Status != CustomerStatus.Preferred
        && total > PreferredThreshold)
    {
        customer.Status = CustomerStatus.Preferred;
        gw.Update(customer);
    }
}

Another potential database write operation, as it turns out - complete with an apology. Now that we've learned all about the details of the code, even the GetOpenOrders method is beginning to look suspect. The GetOrders method returns all orders, with the side effect that all orders were audited as having been read by the user, but the GetOpenOrders filters the output. In the end, it turns out that we can't even trust the audit trail.

While I must apologize for this contrived example of a Transaction Script, it's clear that when code looks like that, it's no wonder why developers think that it's necessary to contain the entire code base in their heads. When this is the case, interfaces are only in the way.

However, this is not the fault of loose coupling, but rather a failure to adhere to the very fundamental principle of Command-Query Separation (CQS). You should be able to tell from the method signature alone whether invoking the method will or will not have side-effects. This is one of the key messages from Clean Code: the method name and signature is an abstraction. You should be able to reason about the behavior of the method from its declaration. You shouldn't have to read the code to get an idea about what it does.

Abstractions always hide details. Method declarations do too. The point is that you should be able to read just the method declaration in order to gain a basic understanding of what's going on. You can always return to the method's code later in order to understand detail, but reading the method declaration alone should provide the Big Picture.

Strictly adhering to CQS goes a long way in enabling you to understand a loosely coupled code base. If you can reason about methods at a high level, you don't need to see “the other side of the interface” in order to understand the Big Picture.

Stack Traces #

Still, even in a loosely coupled code base with great test coverage, integration issues arise. While each class works fine in isolation, when you integrate them, sometimes the interactions between them cause errors. This is often because of incorrect assumptions about the collaborators, which often indicates that the LSP was somehow violated.

To understand why such errors occur, we need to understand which concrete classes are interacting. How do we do that in a loosely coupled code base?

That's actually easy: look at the stack trace from your error report. If your error report doesn't include a stack trace, make sure that it's going to do that in the future.

The stack trace is one of the most important troubleshooting tools in a loosely coupled code base, because it's going to tell you exactly which classes were interacting when an exception was thrown.

Furthermore, if the code base also adheres to the Single Responsibility Principle and the ideals from Clean Code, each method should be very small (under 15 lines of code). If that's the case, you can often understand the exact nature of the error from the stack trace and the error message alone. It shouldn't even be necessary to attach a debugger to understand the bug, but in a pinch, you can still do that.

Tooling #

Returning to the original question, I often hear people advocating tools such as IDE add-ins which support navigation across interfaces. Such tools might provide a menu option which enables you to “go to implementation”. At this point it should be clear that such a tool is mainly going to be helpful in code bases that violate the RAP.

(I'm not naming any particular tools here because I'm not interested in turning this post into a discussion about the merits of various specific tools.)

Conclusion #

It's the responsibility of the loosely coupled code base to make sure that it's easy to understand the Big Picture and that it's easy to work with. In the end, that responsibility falls on the developers who write the code - not the developer who's trying to understand it.

Comments

SOLID is a set of principles that, if applied consistently, has some surprising effect on code. In a previous post I provided a sketch of what it means to meticulously apply the Single Responsibility Principle. In this article I will describe what happens when you follow the Open/Closed Principle (OCP) to its logical conclusion.

In case a refresher is required, the OCP states that a class should be open for extension, but closed for modification. It seems to me that people often forget the second part. What does it mean?

It means that once implemented, you shouldn't touch that piece of code ever again (unless you need to correct a bug).

Then how can new functionality be added to a code base? This is still possible through either inheritance or polymorphic recomposition. Since the L in SOLID signifies the Liskov Substitution Principle, SOLID code tends to be based on loosely coupled code composed into an application through copious use of interfaces - basically, Strategies injected into other Strategies and so on (also due to Dependency Inversion Principle). In order to add functionality, you can create new implementations of these interfaces and redefine the application's Composition Root. Perhaps you'd be wrapping existing functionality in a Decorator or adding it to a Composite.

Once in a while, you'll stop using an old implementation of an interface. Should you then delete this implementation? What would be the point? At a certain point in time, this implementation was valuable. Maybe it will become valuable again. Leaving it as an potential building block seems a better choice.

Thus, if we think about working with code as a CRUD endeavor, SOLID code can be Created and Read, but never Updated or Deleted. In other words, true SOLID code is append-only code.

Example: Changing AutoFixture's Number Generation Algorithm #

In early 2011 an issue was reported for AutoFixture: Anonymous numbers were created in monotonically increasing sequences, but with separate sequences for each number type:

integers: 1, 2, 3, 4, 5, …

decimals: 1.0, 2.0, 3.0, 4.0, 5.0, …

and so on. However, the person reporting the issue thought it made more sense if all numbers shared a single sequence. After thinking about it a little while, I agreed.

Because the AutoFixture code base is fairly SOLID we decided to leave the old implementations in place and implement the new behavior in new classes.

The old behavior was composed from a set of ISpecimenBuilders. As an example, integers were generated by this class:

public class Int32SequenceGenerator : ISpecimenBuilder
{
    private int i;
 
    public int CreateAnonymous()
    {
        return Interlocked.Increment(ref this.i);
    }
 
    public object Create(object request,
        ISpecimenContext context)
    {
        if (request != typeof(int))
        {
            return new NoSpecimen(request);
        }
 
        return this.CreateAnonymous();
    }
}

Similar implementations generated decimals, floats, doubles, etc. Instead of modifying any of these classes, we left them in the code base and created a new ISpecimenBuilder that generates all numbers from a single sequence:

public class NumericSequenceGenerator : ISpecimenBuilder
{
    private int value;
 
    public object Create(object request,
        ISpecimenContext context)
    {
        var type = request as Type;
        if (type == null)
            return new NoSpecimen(request);
 
        return this.CreateNumericSpecimen(type);
    }
 
    private object CreateNumericSpecimen(Type request)
    {
        var typeCode = Type.GetTypeCode(request);
 
        switch (typeCode)
        {
            case TypeCode.Byte:
                return (byte)this.GetNextNumber();
            case TypeCode.Decimal:
                return (decimal)this.GetNextNumber();
            case TypeCode.Double:
                return (double)this.GetNextNumber();
            case TypeCode.Int16:
                return (short)this.GetNextNumber();
            case TypeCode.Int32:
                return this.GetNextNumber();
            case TypeCode.Int64:
                return (long)this.GetNextNumber();
            case TypeCode.SByte:
                return (sbyte)this.GetNextNumber();
            case TypeCode.Single:
                return (float)this.GetNextNumber();
            case TypeCode.UInt16:
                return (ushort)this.GetNextNumber();
            case TypeCode.UInt32:
                return (uint)this.GetNextNumber();
            case TypeCode.UInt64:
                return (ulong)this.GetNextNumber();
            default:
                return new NoSpecimen(request);
        }
    }
 
    private int GetNextNumber()
    {
        return Interlocked.Increment(ref this.value);
    }
}

Adding a new class in itself has no effect, so in order to recompose the default behavior of AutoFixture, we changed a class called DefaultPrimitiveBuilders by removing the old ISpecimenBuilders like Int32SequenceGenerator and instead adding NumericSequenceGenerator:

yield return new StringGenerator(() => 
    Guid.NewGuid());
yield return new ConstrainedStringGenerator();
yield return new StringSeedRelay();
yield return new NumericSequenceGenerator();
yield return new CharSequenceGenerator();
yield return new RangedNumberGenerator();
// even more builders...

NumericSequenceGenerator is the fourth class being yielded here. Before we added NumericSequenceGenerator, this class instead yielded Int32SequenceGenerator and similar classes. These were removed.

The DefaultPrimitiveBuilders class is part of AutoFixture's default Facade and is the closest we get to a Composition Root for the library. Recomposing this Facade enabled us to change the behavior of AutoFixture without modifying (other) existing classes.

As Enrico (who implemented this change) points out, the beauty is that the previous behavior is still in the box, and all it takes is a single method call to bring it back:

var fixture = new Fixture().Customize(
    new NumericSequencePerTypeCustomization());

The only class we had to modify was the DefaultPrimitiveBuilders, which is where the object graph is composed. In applications this corresponds to the Composition Root, so even in the face of SOLID code, you still need to modify the Composition Root in order to recompose the application. However, use of a good DI Container and a strong set of conventions can do much to minimize the required editing of such a class.

SOLID versus Refactoring #

SOLID is a goal I strive towards in the way I write code and design APIs, but I don't think I've ever written a significant code base which is perfectly SOLID. While I consider AutoFixture a ‘fairly' SOLID code base, it's not perfect, and I'm currently performing some design work in order to change some abstractions for version 3.0. This will require changing some of the existing types and thereby violating the OCP.

It's worth noting that as long as you can stick with the OCP you can avoid introducing breaking changes. A breaking change is also an OCP violation, so adhering to the OCP is more than just an academic exercise - particularly if you write reusable libraries.

Still, while none of my code is perfect and I occasionally have to refactor, I don't refactor much. By definition, refactoring means violating the OCP, and while I have nothing against refactoring code when it's required, I much prefer putting myself in a situation where it's rarely necessary in the first place.

I've often been derided for my lack of use of Resharper. When replying that I have little use for Resharper because I write SOLID code and thus don't do much refactoring, I've been ridiculed for being totally clueless. People don't realize the intimate relationship between SOLID and refactoring. I hope this post has helped highlight that connection.

Comments

Here's a question I often get:

“Should I test my DI Container configuration?”

The motivation for asking mostly seems to be that people want to know whether or not their applications are correctly wired. That makes sense.

A related question I also often get is whether or not a particular container has a self-test feature? In this post I'll attempt to answer both questions.

Container Self-testing #

Some DI Containers have a method you can invoke to make it perform a consistency check on itself. As an example, StructureMap has the AssertConfigurationIsValid method that, according to documentation does “a full environment test of the configuration of [the] container.” It will “try to create every configured instance [...]”

Calling the method is really easy:

container.AssertConfigurationIsValid();

Such a self-test can often be an expensive operation (not only for StructureMap, but in general) because it's basically attempting to create an instance of each and every Service registered in the container. If the configuration is large, it's going to take some time, but it's still going to be faster than performing a manual test of the application.

Two questions remain: Is it worth invoking this method? Why don't all containers have such a method?

The quick answer is that such a method is close to worthless, which also explains why many containers don't include one.

To understand the answer, consider the set of all components contained in the container in this figure:

The container contains the set of components IFoo, IBar, IBaz, Foo, Bar, Baz, and Qux so a self-test will try to create a single instance of each of these seven types. If all seven instances can be created, the test succeeds.

All this accomplishes is to verify that the configuration is internally consistent. Even so, an application could require instances of the ICorge, Corge or Grault types which are completely external to the configuration, in which case resolution would fail.

Even more subtly, resolution would also fail whenever the container is queried for an instance of IQux, since this interface isn't part of the configuration, even though it's related to the concrete Qux type which is registered in the container. A self-test only verifies that the concrete Qux class can be resolved, but it never attempts to create an instance of the IQux interface.

In short, the fact that a container's configuration is internally consistent doesn't guarantee that all services required by an application can be served.

Still, you may think that at least a self-test can constitute an early warning system: if the self-test fails, surely it must mean that the configuration is invalid? Unfortunately, that's not true either.

If a container is being configured using Auto-registration/Convention over Configuration to scan one or more assemblies and register certain types contained within, chances are that ‘too many' types will end up being registered - particularly if one or more of these assemblies are reusable libraries (as opposed to application-specific assemblies). Often, the number of redundant types added is negligible, but they may make the configuration internally inconsistent. However, if the inconsistency only affects the redundant types, it doesn't matter. The container will still be able to resolve everything the current application requires.

Thus, a container self-test method is worthless.

Then how can the container configuration be tested?

Explicit Testing of Container Configuration #

Since a container self-test doesn't achieve the desired goal, how can we ensure that an application can be composed correctly?

One option is to write an automated integration test (not a unit test) for each service that the application requires. Still, if done manually, you run the risk of forgetting to write a test for a specific service. A better option is to come up with a convention so that you can identify all the services required by a specific application, and then write a convention-based test to verify that the container can resolve them all.

Will this guarantee that the application can be correctly composed?

No, it only guarantees that it can be composed - not that this composition is correct.

Even when a composed instance can be created for each and every service, many things may still be wrong:

• Composition is just plain wrong:
  • Decorators may be missing
  • Decorators may have been added in the wrong order
  • The wrong services are injected into consumers (this is more likely to happen when you follow the Reused Abstractions Principle, since there will be multiple concrete implementations of each interface)
• Configuration values like connection strings and such are incorrect - e.g. while a connection string is supplied to a constructor, it may not contain the correct values.
• Even if everything is correctly composed, the run-time environment may prevent the application from working. As an example, even if an injected connection string is correct, there may not be any connection to the database due to network or security misconfiguration.

In short, a Subcutaneous Test or full System Test is the only way to verify that everything is correctly wired. However, if you have good test coverage at the unit test level, a series of Smoke Tests is all that you need at the System Test level because in general you have good reason to believe that all behavior is correct. The remaining question is whether all this correct behavior can be correctly connected, and that tends to be an all-or-nothing proposition.

Conclusion #

While it would be possible to write a set of convention-based integration tests to verify the configuration of a DI Container, the return of investment is too low since it doesn't remove the need for a set of Smoke Tests at the System Test level.

Comments

Recently I received a question from Kelly Sommers about good ways to refactor away from Factory Overload. Basically, she's working in a code base where there's an explosion of Abstract Factories which seems to be counter-productive. In this post I'll take a look at the example problem and propose a set of alternatives.

An Abstract Factory (and its close relative Product Trader) can serve as a solution to various challenges that come up when writing loosely coupled code (chapter 6 of my book describes the most common scenarios). However, introducing an Abstract Factory may be a leaky abstraction, so don't do it blindly. For example, an Abstract Factory is rarely the best approach to address lifetime management concerns. In other words, the Abstract Factory has to make sense as a pure model element.

That's not the case in the following example.

Problem Statement #

The question centers around a code base that integrates with a database closely guarded by DBA police. Apparently, every single database access must happen through a set of very fine-grained stored procedures.

For example, to update the first name of a user, a set of stored procedures exist to support this scenario, depending on the context of the current application user:

As this table demonstrates, not only is there a stored procedure for each user context, but the parameter name differs as well. However, in this particular case it seems as though there's a pattern to the names.

If this pattern is consistent, I think the easiest way to address these variations would be to algorithmically build the strings from a couple of templates.

However, this is not the route taken by Kelly's team, so I assume that things are more complicated than that; apparently, a templated approach is not viable, so for the rest of  this article I'm going to assume that it's necessary to write at least some code to address each case individually.

The current solution that Kelly's team has implemented is to use an Abstract Factory (Product Trader) to translate the user type into an appropriate IUserFirstNameModifier instance. From the consuming code, it looks like this:

var modifier = factory.Create(UserTypes.Admin);
modifier.Commit("first");

where the factory variable is an instance of the IUserFirstNameModifierFactory interface. This is certainly loosely coupled, but looks like a leaky abstraction. Why is a factory needed? It seems that its single responsibility is to translate a UserTypes instance (an enum) into an IUserFirstNameModifier. There's a code smell buried here - try to spot it before you read on :)

Proposed Solution #

Kelly herself suggests an alternative involving a concrete Builder which can create instances of a single concrete UserFirstNameModifier with or without an implicit conversion:

// Implicit conversion.
UserFirstNameModifier modifier1 = 
    builder.WithUserType(UserTypes.Guest);
 
// Without implicit conversion.
var modifier2 = builder
    .WithUserType(UserTypes.Restricted)
    .Create();

While this may seem to reduce the number of classes involved, it has several drawbacks:

• First of all, the Fluent Builder pattern implies that you can forgo invoking any of the WithXyz methods (WithUserType) and just accept all the default values encapsulated in the builder. This again implies that there's a default user type, which may or may not make sense in that particular domain. Looking at Kelly's code, UserTypes is an enum (and thus has a default value), so if WithUserType isn't invoked, the Create method defaults to UserTypes.Admin. That's a bit too implicit for my taste.
• Since all involved classes are now concrete, the proposed solution isn't extensibile (and by corollary hard to unit test).
• The builder is essentially a big switch statement.

Both the current implementation and the proposed solution involves passing an enum as a method parameter to a different class. If you've read and memorized Refactoring you should by now have recognized both a code smell and the remedy.

Alternative 1a: Make UserType Polymorphic #

The code smell is Feature Envy and a possible refactoring is to replace the enum with a Strategy. In order to do that, an IUserType interface is introduced:

public interface IUserType
{
    IUserFirstNameModifer CreateUserFirstNameModifier();
}

Usage becomes as simple as this:

var modifier = userType.CreateUserFirstNameModifier();

Obviously, more methods can be added to IUserType to support other update operations, but care should be taken to avoid creating a Header Interface.

While this solution is much more object-oriented, I'm still not quite happy with it, because apparently, the context is a CQRS style architecture. Since an update operation is essentially a Command, then why model the implementation along the lines of a Query? Both Abstract Factory and Factory Method patterns represent Queries, so it seems redundant in this case. It should be possible to apply the Hollywood Principle here.

Alternative 1b: Tell, Don't Ask #

Why have the user type return an modifier? Why can't it perform the update itself? The IUserType interface should be changed to something like this:

public interface IUserType
{
    void CommitUserFirtName(string firstName);
}

This makes it easier for the consumer to commit the user's first name because it can be done directly on the IUserType instance instead of first creating the modifier.

It also makes it much easier to unit test the consumer because there's no longer a mix of Command and Queries within the same method. From Growing Object-Oriented Software we know that Queries should be modeled with Stubs and Commands with Mocks, and if you've ever tried mixing the two you know that it's a sort of interaction that should be minimized.

Alternative 2a: Distinguish by Type #

While I personally like alternative 1b best, it may not be practical in all situations, so it's always valuable to examine other alternatives.

The root cause of the problem is that there's a lot of stored procedures. I want to reiterate that I still think that the absolutely easiest solution would be to generate a SqlCommand from a string template, but given that this article assumes that this isn't possible or desirable, it follows that code must be written for each stored procedure.

Why not simply define an interface for each one? As an example, to update the user's first name in the context of being an ‘Admin' user, this Role Interface can be used:

public interface IUserFirstNameAdminModifier
{
    void Commit(string firstName);
}

Similar interfaces can be defined for the other user types, such as IUserFirstNameRestrictedModifier, IUserFirstNameGuestModifier and so on.

This is a very simple solution; it's easy to implement, but risks violating the Reused Abstractions Principle (RAP).

Alternative 2b: Distinguish by Generic Type #

The problem with introducing interfaces like IUserFirstNameAdminModifier, IUserFirstNameRestrictedModifier, IUserFirstNameGuestModifier etc. is that they differ only by name. The Commit method is the same for all these interfaces, so this seems to violate the RAP. It'd be better to merge all these interfaces into a single interface, which is what Kelly's team is currently doing. However, the problem with this is that the type carries no information about the role that the modifier is playing.

Another alternative is to turn the modifier interface into a generic interface like this:

public interface IUserFirstNameModifier<T> 
    where T : IUserType
{
    void Commit(string firstName);
}

The IUserType is a Marker Interface, so .NET purists are not going to like this solution, since the .NET Type Design Guidelines recommend against using Marker Interfaces. However, it's impossible to constrain a generic type argument against an attribute, so the party line solution is out of the question.

This solution ensures that consumers can now have dependencies on IUserFirstNameModifier<AdminUserType>, IUserFirstNameModifier<RestrictedUserType>, etc.

However, the need for a marker interface gives off another odeur.

Alternative 3: Distinguish by Role #

The problem at the heart of alternative 2 is that it attempts to use the type of the interfaces as an indicator of the roles that Services play. It's seems that making the type distinct works against the RAP, but when the RAP is applied, the type becomes ambiguous.

However, as Ted Neward points out in his excellent series on Multiparadigmatic .NET, the type is only one axis of variability among many. Perhaps, in this case, it may be much easier to use the name of the dependency to communicate its role instead of the type.

Given a single, ambiguous IUserFirstNameModifier interface (just as in the original problem statement), a consumer can distinguish between the various roles of modifiers by their names:

public partial class SomeConsumer
{
    private readonly IUserFirstNameModifier adminModifier;
    private readonly IUserFirstNameModifier guestModifier;
 
    public SomeConsumer(
        IUserFirstNameModifier adminModifier,
        IUserFirstNameModifier guestModifier)
    {
        this.adminModifier = adminModifier;
        this.guestModifier = guestModifier;
    }
 
    public void DoSomething()
    {
        if (this.UseAdmin)
            this.adminModifier.Commit("first");
        else
            this.guestModifier.Commit("first");
    }
}

Now it's entirely up to the Composition Root to compose SomeConsumer with the correct modifiers, and while this can be done manually, it's an excellent case for a DI Container and a bit of Convention over Configuration.

Conclusion #

I'm sure that if I'd spent more time analyzing the problem I could have come up with more alternatives, but this post is becoming long enough already.

Of the alternatives I've suggested here, I prefer 1b or 3, depending on the exact requirements.

Comments