Separation of Concerns by example: Part 4

In the last part, we finally broke out the caching and data access concerns from our original class.  The series so far includes:

• Separation of Concerns – how not to do it
• Separation of Concerns by example: Part 1 – Refactoring away from static class
• Separation of Concerns by example: Part 2 – Specialized interface for Cache
• Separation of Concerns by example: Part 3 – Creating the repository

Things are looking up, we’ve broken out the dependencies into distinct concerns, including caching, data access and finding/paging.  Unfortunately, our high-volume site has run into some intermittent issues.  Every now and then, especially during peak hours, the application experiences NullReferenceExceptions, coming from our new and (hopefully) improved CustomerFinder.

###

Finding the defect

Let’s review the current CustomerFinder to see how we might run into a NullReferenceException:

public class CustomerFinder
{
    private readonly ICustomerRepository _customerRepository;
    private readonly ICustomCache _customCache;

    public CustomerFinder(ICustomerRepository customerRepository, ICustomCache customCache)
    {
        _customerRepository = customerRepository;
        _customCache = customCache;
    }

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

        if (_customCache.Contains(key))
        {
            customers = _customCache.GetItem<Customer[]>(key);
        }
        else
        {
            customers =_customerRepository
                        .FindAllCustomers()
                        .Skip(startRowIndex)
                        .Take(maximumRows)
                        .ToArray();

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

        return customers;
    }

}

Somehow, our FindAllCustomers method is returning a null Customer array, causing us to break our contract with callers of this class.  Anything that returns an array should never return null, only an empty array if no items are returned.

So how could the return value be null?  Look at the CustomerFinder class, we see it’s initialized to null, and assigned twice.  Once in the caching scenario, and once in the repository scenario.  The repository scenario doesn’t seem to fit, as the ToArray extension method never returns null.

Looking at the caching scenario, combined with our knowledge of when the bug happens, it looks like we might have our culprit. Suppose that under heavy load, the cache item expires between the Contains and GetItem call?  GetItem could certainly return null in that case.  Looking at the Cache documentation, getting an expired (and therefore non-existing) item does not throw any exception, and returns null.

Let’s look at changing the behavior so that we take this scenario into account.

Making the change (safely)

Since we’d like to minimize the risk of change to this piece of code (after all, we have a high volume website to keep going), we’ll add a test to cover the behavior we want to see, before we make the change.  After all, if the behavior we specify should exist, already does, then either our assumptions are wrong or our code already works.

First, let’s think of the context of when this bug happens.  It occurred when the cache item expired after I checked the cache.  Let’s create our specification with that as the description:

public class When_finding_customers_and_the_cache_expires_after_checking_the_cache 
    : ContextSpecification
{
    
}

This is basically a Testcase Class per Feature xUnit pattern, with a BDD-style naming.  Basically, I’m describing the context/scenario where the behavior I want to observe is valid.  The ContextSpecification is similar version of JP’s base specification class that I’m using from my pet project, NBehave.

Now we need to describe the behavior we want to see.  I’d like to see a few things happen:

• The customers returned shouldn’t be null
• It should use the repository to find the customers (as the cache didn’t work)
• It should push the results back into the cache

All of these I might write down in an index card or something.  It’s something that helps me think through the behaviors I’m trying to specify.  Here’s the test class with the behaviors specified:

public class When_finding_products_and_the_cache_expires_after_checking_the_cache 
    : ContextSpecification
{
    [Test]
    public void Customers_returned_should_not_be_null()
    {
    }

    [Test]
    public void It_should_use_the_repository_to_find_the_customers()
    {
    }

    [Test]
    public void Customers_returned_should_be_stored_in_the_cache()
    {
    }
}

With Rhino Mocks 3.5, testing these indirect inputs and outputs is a snap.  Now that we have our specifications, let’s establish the context, the reason for the behaviors, and fill out our verification of the desired behaviors:

public class When_finding_products_and_the_cache_expires_after_checking_the_cache 
    : ContextSpecification
{
    private CustomerFinder _customerFinder;
    private Customer[] _actualCustomers;
    private ICustomerRepository _customerRepository;
    private ICustomCache _customCache;

    protected override void EstablishContext()
    {
        _customCache = Dependency<ICustomCache>();
        _customerRepository = Dependency<ICustomerRepository>();

        _customerFinder = new CustomerFinder(_customerRepository, _customCache);

        var customer1 = new Customer {CustomerID = "1"};
        var customer2 = new Customer {CustomerID = "2"};

        var foundCustomers = new[]
                                 {
                                     customer1,
                                     customer2
                                 }
                                 .AsQueryable()
                                 .OrderBy(cust => cust.CustomerID);

        _customCache.Stub(cache => cache.Contains("Customers_Customers_1_1")).Return(true);
        _customCache.Stub(cache => cache.GetItem<Customer[]>("Customers_Customers_1_1")).Return(null);
        _customerRepository.Stub(repo => repo.FindAllCustomers()).Return(foundCustomers);
    }

    protected override void Because()
    {
        _actualCustomers = _customerFinder.FindAllCustomers(1, 1);
    }

    [Test]
    public void Customers_returned_should_not_be_null()
    {
        _actualCustomers.ShouldNotBeNull();
    }

    [Test]
    public void It_should_use_the_repository_to_find_the_customers()
    {
        _customerRepository.AssertWasCalled(repo => repo.FindAllCustomers());
    }

    [Test]
    public void Customers_returned_should_be_stored_in_the_cache()
    {
        _customCache.AssertWasCalled(cache => cache.AddItemExpringTomorrow<Customer[]>(null, null),
            option => option.Constraints(Is.Equal("Customers_Customers_1_1"), 
                                         Is.Matching<Customer[]>(item => item.Length == 1)));
    }

}

With Rhino Mocks new Arrange Act Assert syntax, it’s much easier to separate the specifications into the context, reason, and verification of the behavior.  In TDD terms, I’ve separated the Setup, Exercise, and Assert into three different parts.  Each behavior is verified independently of the other.

Running my specification through TestDriven.NET tells me that all three behaviors don’t work yet, and I need to modify my CustomerFinder to get them green again.  Here’s my CustomerFinder after the changes:

public class CustomerFinder
{
    private readonly ICustomerRepository _customerRepository;
    private readonly ICustomCache _customCache;

    public CustomerFinder(ICustomerRepository customerRepository, ICustomCache customCache)
    {
        _customerRepository = customerRepository;
        _customCache = customCache;
    }

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

        if (_customCache.Contains(key))
        {
            customers = _customCache.GetItem<Customer[]>(key);
        }

        if (customers == null)
        {
            customers =_customerRepository
                        .FindAllCustomers()
                        .Skip(startRowIndex)
                        .Take(maximumRows)
                        .ToArray();

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

        return customers;
    }

}

Here, I broke out the if/else into two if statements, where the repository search would still occur if the cache retrieval fails.  Going back to my specification, I see that all behaviors are working now.  It looks like our fix is ready to go into production.

###

Quick Review

In the original CustomerFinder, this kind of behavioral specification would have been impossible because of the coupling with ASP.NET Cache and LINQ to SQL.  By breaking out dependencies, I was able to make changes safely to the existing behavior by creating specifications that could mock and stub out any test doubles.  To do so, we:

• Created a test fixture, naming it after the context of our bug
• Added specification methods for the behavior we wanted to observe
• Filled out the context, reason, and verification of the desired behavior

If the dependencies hadn’t been broken out, I would have been in the “Edit and Pray” mode of changing code.  This kind of bug only shows up in production, so it could be quite expensive if I had to make a blind change (and I was wrong).  Breaking the dependencies out allows me to use Test Doubles in place of the real dependencies, allowing me to tweak the indirect inputs and outputs to my CustomerFinder.

Next time, we’ll look at how we can better manage the creation and dependency lookup of our CustomerFinder through an Inversion of Control container.