Separation of Concerns by example: Part 2

Separation of concerns is one of the fundamental tenets of good object-oriented design.  Anyone can throw a bunch of code in a method and call it a day, but that’s not the most maintainable approach.  So far, we’ve looked at:

In this part, I’d like to look at removing the dependency on HttpContext.  Here’s what our classes look like thus far:

public class CustomerManager
{
    [DataObjectMethod(DataObjectMethodType.Select, false)]
    public static List<Customer> GetCustomers(int startRowIndex, int maximumRows)
    {
        var finder = new CustomerFinder();
        return new CustomerFinder().FindAllCustomers(startRowIndex, maximumRows);
    }
}

public class CustomerFinder
{
    public List<Customer> FindAllCustomers(int startRowIndex, int maximumRows)
    {
        List<Customer> customers = null;
        string key = "Customers_Customers_" + startRowIndex + "_" + maximumRows;

        if (HttpContext.Current.Cache[key] != null)
        {
            customers = (List<Customer>) HttpContext.Current.Cache[key];
        }
        else
        {
            customers =
                (
                    from
                        c in DataGateway.Context.Customers
                    orderby
                        c.CustomerID descending
                    select
                        c
                ).Skip(startRowIndex).Take(maximumRows).ToList();

            if ((customers != null) && (customers.Count > 0))
                HttpContext.Current.Cache.Insert(key, customers, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
        }

        return customers;
    }
}

The CustomerManager class is our presentation-layer service, and is only a very thin wrapper on top of the domain and application services.  We want to push the important business logic down into the heart of the domain as much as possible, where it belongs.

The goal is to write a single passing test for the CustomerFinder class.  Right now, that isn’t possible because of the dependencies on HttpContext and DataGateway.

For now, the immediate concern has to be HttpContext.  Having a dependency directly on Linq to SQL is bad, but at least the test can run without it.  It’s not so simple to remove the HttpContext dependency, as we have quite a few choices on how to do so.

Designing the dependency

We know we’re going to use constructor injection for this dependency.  What that means is that we’ll use an interface to act as a facade, and the CustomerFinder will use that interface to do its work.

Under test, we’ll pass a fake instance to test the indirect inputs and outputs to the test, while at runtime, the “real” instance will be used.

So the basic idea will be to hide the caching part behind an interface.  But what should we hide?  We have three options:

  • Create an IHttpContext interface to hide HttpContext.Current
  • Create an ICache interface to hide the HttpContext.Current.Cache property
  • Create a specialized interface to hide the entire cache calls

Abstracting HttpContext.Current

This is a huge rabbit hole we don’t want to go down.  HttpContext is a very large class with a ton of methods and properties.  We could create an IHttpContext interface that only has one property for Cache, but that has its own issues, too.

The Cache class is sealed, meaning we can’t subclass it to create a fake instance.  So we’d have to create an ICache implementation anyway to get around this limitation.  And at this point, what is IHttpContext giving us other than a window to the Cache?  Nothing.

Some MVC frameworks opt to hide the entire HttpContext behind an interface or an abstract class.  These still violate the Interface Segregation Principle, as implementers of the interface have to provide implementations of Request, Response, Session, Cookies, etc etc.

This isn’t a good choice, but what about a targeted Cache implementation?

Abstracting Cache

This looks like a better choice, as the Cache class is much smaller and lighter and HttpContext.  However, it still suffers from similar problems as the IHttpContext, where implementers have to know a great deal about hows and whys of Cache to get it to work properly.

One other issue an ICache implementation won’t solve is the non-intention-revealing interface the Cache.Insert method contains. It’s difficult to read the code inside CustomerFinder to see what the caching strategy is.

With that strategy out, we’re down to our final option.

Specialized interface

Specialized interface entails creating an interface that only hides what we use, and providing use-specific names for the methods.  Instead of Cache.Insert, we might have Cache.InsertItemExpiringTomorrow.  It’s a much more explicit interface, describing the intent of what our caching logic is doing.

Creating the specialized interface

As always, we’ll first want to extract our methods out of the CustomerFinder class, providing good names for each of the methods.  We’ll need to extract both the section that peeks inside Cache, retrieves an item from Cache, and inserts an item.  Along the way, we want to make sure we use the same names as what our interface will provide, which will save a rename step later.

After the Extract Method refactorings, our class now looks like this:

public class CustomerFinder
{
    public List<Customer> FindAllCustomers(int startRowIndex, int maximumRows)
    {
        List<Customer> customers = null;
        string key = "Customers_Customers_" + startRowIndex + "_" + maximumRows;

        if (Contains(key))
        {
            customers = GetItem(key);
        }
        else
        {
            customers =
                (
                    from
                        c in DataGateway.Context.Customers
                    orderby
                        c.CustomerID descending
                    select
                        c
                ).Skip(startRowIndex).Take(maximumRows).ToList();

            if ((customers != null) && (customers.Count > 0))
                AddItemExpringTomorrow(key, customers);
        }

        return customers;
    }

    private void AddItemExpringTomorrow(string key, List<Customer> customers)
    {
        HttpContext.Current.Cache.Insert(key, customers, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
    }

    private List<Customer> GetItem(string key)
    {
        return (List<Customer>) HttpContext.Current.Cache[key];
    }

    private bool Contains(string key)
    {
        return HttpContext.Current.Cache[key] != null;
    }
}

But this isn’t quite right, is it?  We have some problems already:

  • The Insert and Get methods are coupled to the List<Customer> type.  We’re not making an ICustomersCache, so generics will help here.

After fixing the changes above, our extracted methods now look like this:

private void AddItemExpringTomorrow<T>(string key, T item)
{
    HttpContext.Current.Cache.Insert(key, item, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
}

private T GetItem<T>(string key)
{
    return (T) HttpContext.Current.Cache[key];
}

private bool Contains(string key)
{
    return HttpContext.Current.Cache[key] != null;
}

That’s much better.  Now that we have a group of methods with our Cache logic we need, we can perform Extract Class to create our specialized Cache implementation.  All we’ll need to do is create a CustomCache class and move these methods to that class:

public class CustomCache
{
    public void AddItemExpringTomorrow<T>(string key, T item)
    {
        HttpContext.Current.Cache.Insert(key, item, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
    }

    public T GetItem<T>(string key)
    {
        return (T)HttpContext.Current.Cache[key];
    }

    public bool Contains(string key)
    {
        return HttpContext.Current.Cache[key] != null;
    }
}

Our CustomerFinder class isn’t compiling, but we have one more step for the CustomCache class before it’s ready for prime time.  We need to Extract Interface so that our CustomerFinder implementation has something less concrete to work with.  ReSharper lets us extract the interface easily, which leaves us with our final CustomCache implementation:

public interface ICustomCache
{
    void AddItemExpringTomorrow<T>(string key, T item);
    T GetItem<T>(string key);
    bool Contains(string key);
}

public class CustomCache : ICustomCache
{
    public void AddItemExpringTomorrow<T>(string key, T item)
    {
        HttpContext.Current.Cache.Insert(key, item, null, DateTime.Now.AddDays(1), TimeSpan.Zero);
    }

    public T GetItem<T>(string key)
    {
        return (T)HttpContext.Current.Cache[key];
    }

    public bool Contains(string key)
    {
        return HttpContext.Current.Cache[key] != null;
    }
}

Now the ICustomCache is something that the CustomerFinder can deal with.  Since we’re using constructor injection, we’ll want to create a constructor that takes an ICustomCache implementation.  Our CustomerFinder will now look like:

public class CustomerFinder
{
    private readonly ICustomCache _customCache;

    public CustomerFinder(ICustomCache customCache)
    {
        _customCache = customCache;
    }

    public List<Customer> FindAllCustomers(int startRowIndex, int maximumRows)
    {
        List<Customer> customers = null;
        string key = "Customers_Customers_" + startRowIndex + "_" + maximumRows;

        if (_customCache.Contains(key))
        {
            customers = _customCache.GetItem<List<Customer>>(key);
        }
        else
        {
            customers =
                (
                    from
                        c in DataGateway.Context.Customers
                    orderby
                        c.CustomerID descending
                    select
                        c
                ).Skip(startRowIndex).Take(maximumRows).ToList();

            if ((customers != null) && (customers.Count > 0))
                _customCache.AddItemExpringTomorrow(key, customers);
        }

        return customers;
    }

}

Our CustomerFinder now has no opaque dependency on the Cache or HttpContext classes.  We only deal with our wrapper, which has more explicit names about what we’re trying to do.

But now our CustomerManager class isn’t compiling, as we removed the no-argument constructor for CustomerFinder.  In this case, I’ll just allow the CustomerManager to instantiate the correct implementations of ICustomCache:

public class CustomerManager
{
    [DataObjectMethod(DataObjectMethodType.Select, false)]
    public static List<Customer> GetCustomers(int startRowIndex, int maximumRows)
    {
        var finder = new CustomerFinder(new CustomCache());
        return finder.FindAllCustomers(startRowIndex, maximumRows);
    }
}

We could have gone several ways on that one, from a creation method, to a factory, all the way to an IoC container like Spring or StructureMap.  We’ll wait on any changes like that.

Quick review

Our CustomerFinder class had an opaque dependency on HttpContext.Current and Cache.  We looked at a few options at making that dependency explicit through constructor injection, settling on a specialized implementation that contained intention-revealing names for only the operations we support.  To do so, we performed a number of refactorings:

Finally, we fixed the CustomerManager class to use our new constructor with the appropriate dependency.  The maintainability of the CustomerFinder class has improved significantly, as well as usability, as both users and maintainers of this class can see immediately the explicit dependencies this class requires.

Next time, we’ll look at getting rid of that pesky Linq to SQL dependency.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in Refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://flimflan.com/blog Joshua Flanagan

    Did you intend to rename the Contains method? Right now, it reads like “if custom cache does NOT contain the item, retrieve the item from the custom cache.”

  • http://davesquared.blogspot.com David

    This is a great series Jimmy! Looking forward to the next one :)

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Josh

    Yeah you’re right. I’ll fix that one.

  • Chris Martin

    Good start. In my mind, CustomerFinder is still responsible for too much. Creating a cache key; for instance.

    I like ICustomCache but, instead of a CustomCache (sounds all encompassing), it seems that a CustomerCache would make more sense in this case.

    Now, what about DataGateway? ;)

  • http://telerikwatch.com Todd

    Great series. Don’t forget to reveal the all important examples of how your new classes enable easier testing. I think people will really benefit from seeing how these refactorings enable red/green testing.

  • http://colinjack.blogspot.com Colin Jack

    “Specialized interface entails creating an interface that only hides what we use, and providing use-specific names for the methods. Instead of Cache.Insert, we might have Cache.InsertItemExpiringTomorrow. It’s a much more explicit interface, describing the intent of what our caching logic is doing.”

    Great stuff, I’ve read so many articles on these topics that missed the importance of choosing good abstractions so this was very refreshing.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Chris

    So how would you change the abstraction? Since this caching implementation is not specific to Customers, I’m not sure where it would go.

  • http://jimmybogard.lostechies.com Jimmy Bogard

    @Todd

    Yeah that’s a good idea. It’s what we’re trying to enable. I’ll put some in on the next part.

    @Colin

    Part of the reason I didn’t like the original example were the bad names. “CustomerManager”, etc. Good abstractions are tough, and it’s easier just to pick bad ones.

  • peeles

    +1 @Todd

  • http://iridescence.no Fredrik

    Great stuff!

    “Next time, we’ll look at getting rid of that pesky Linq to SQL dependency.”

    I’ve written a post about creating an abstracting for Linq 2 Sql that you might find interesting: http://www.iridescence.no/Posts/Linq-to-Sql-Programming-Against-an-Interface-and-the-Repository-Pattern.aspx

  • http://www.e-Crescendo.com jdn

    This is awesome stuff.

    Still would be cool as an article submission but I’m printing this out as reference material as you go along.

  • http://blog.naxsoft.com Iouri Goussev

    And the next step will be to further refactor the code to separate cache logic into separate cache management aspect (hint AOP).

  • http://blechie.com/wpierce/ Bill Pierce

    Old post I know, but I would extract an ICustomerFinder interface, and have a decorated CachedCustomerFinder that takes a constructor arg of ICustomerFinder. This separates all cache related functions into the CachedCustomerFinder, allows you to use the CustomerFinder without caching, makes the CacheCustomerFinder easier to test, and allows you to replace the underlying ICustomerFinder with another implementation.

    My (present value) two cents.

    -Bill