How to explain design decisions when refactoring code

Question

I am working on a colleague to refactor the following code, I am trying to turn this into a teaching experience as well.

Original Code

public class WidgetRepository
{
    public Widget GetWidget(int id)
    {
        Widget widget;

        if (HttpContext.Current != null)
        {
            widget = (Widget)HttpContext.Current.Cache["widget_" + id];
        }

        if (widget == null)
        {
            widget = GetWidgetImpl(id); // some db call

            if (HttpContext.Current != null)
            {
                HttpContext.Current.Cache["widget_" + id] = widget;
            }
        }           

        return widget;
    }
}

The most obvious thing that sticks out is that you have your repository knowing about ASP.NET concepts, which is particularly bad as this repository is used by non-ASP.NET code in some scenarios.

Refactored Code

I decided to refactor the code as follows:

public class WidgetRepository
{
    private readonly ICacheProvider _cacheProvider;

    public WidgetRepository(ICacheProvider cacheProvider)
    {
        _cacheProvider = cacheProvider;
    }

    public Widget GetWidget(int id)
    {       
        var widget = _cacheProvider.Get<Widget>("widget_" + id);

        if (widget == null)
        {
            widget = GetWidgetImpl(id); // some db call

            _cacheProvider.Add("widget_" + id, widget);
        }

        return widget;
    }
}

public interface ICacheProvider 
{
    T Get<T>(string key);
    void Add<T>(string key, T item);
}

public HttpCacheProvider : ICacheProvider
{
    private readonly Cache _cache;

    public HttpCacheProvider(Cache cache)
    {
        _cache = cache;
    }

    public T Get<T>(string key)
    {
        return (T)_cache[key];
    }

    public void Add<T>(string key, T item)
    {
        _cache[key] = item;
    }       
}

public NullCacheProvider : ICacheProvider
{
    public T Get<T>(string key)
    {
        return null;
    }

    public void Add<T>(string key, T item)
    {
    }
}

My Justification

I explained the following benefits to the refactored code:

1. Promotes Open / Close: we can change the caching implementation if we want to use something else in the future (memcached, etc.)

2. Lower cyclomatic complexity: less if statements

3. Not mixing ASP.NET technology with data-access technology

Refutations / Concerns

However, I got the following refutations:

1. We're not going to use a different caching technology in the foreseeable future.

2. It's more lines of code (therefore it's more complex)

3. You need to setup the dependencies (complexity is moved, not removed)

4. There's already tonnes of code that uses HttpContext.Current, so this code does not follow the existing (anti-)pattern

5. We're still "using" ASP.NET within data-access code.

My Questions

Personally, I find that the pros outweigh the refutations, however, I would like to be able to address the refutations in order to help educate them.

My questions are:

1. Am I justified in making the refactoring (assuming we have time allocated for cleaning up code)?

2. How do I respond to the various refutations?

Answer 1

Am I justified in making the refactoring (assuming we have time allocated for cleaning up code)?

Are you doing anything else in this code?

If not, this seems not worth the effort. Your peer is likely right that you aren't ever going to use other things - and that you should keep things consistent.

If you are, then cleaning this stuff up too seems like a clear benefit for minimal effort/risk (though you might need to add a default constructor that uses the HttpContext to make it minimal impact).

How do I respond to the various refutations?

1. Yes, but that's no reason to couple something unnecessarily when it's so easy to do things better.

2. More code is not necessarily more complex. In this case, it is more complex, but trivially so. And in exchange, it makes unit testing much easier and the code more flexible.

3. Sure, it's just shifting the problem around, but it's shifting the problem to a more decoupled form. It lets you deal with parts of the problem in isolation rather than having a larger more unweildy problem.

4. "This is what we have always done" is not an excuse to perpetuate bad code/design/anything. Yes, you should aim to keep code consistent, but in this case you should push for consistency with the decoupled pattern rather than the direct HttpContext access. Yes, that's more work - but it's the right thing to do. If your peer pushes that there's not time for all that now (and has an alternative that is better) then maybe agree to do it later (and add a ticket/bug/story to actually do it).

5. See #4.

Answer 2

There's more than one way to skin a cat. Try again until you find a solution that resolves everyone's concerns. Part of the problem is you haven't really broken the coupling on the caching code all the way. I would start with something like this:

public class WidgetRepository
{
  public Widget GetWidget(int id)
  {
    return Cache.GetOrCreate<Widget>("widget_" + id, () => GetWidgetImpl(id));
  }
}

public static class Cache
{
  public static T GetOrCreate<T>(string key, Func<T> createFunc)
  {
    if (HttpContext.Current == null)
    {
      return createFunc();
    }

    T result = (T)HttpContext.Current.Cache[key];

    if (result == null)
    {
      result = createFunc();
      HttpContext.Current.Cache[key] = result;
    }           

    return result;
  }
}

I don't know C#, so there may be a cleaner way to accomplish this same idea (and please ignore syntax problems), but you get the point. Here the lines of code are comparable, but I've separated the caching concern out into a function call, one that should be highly reusable if this pattern repeats as often as you've implied. It follows YAGNI on ever needing a different kind of cache, yet provides a separate place for making that change.

If you really need a separate repository with no connection to caching at all, you can make it a parent class of WidgetRepository. If you ever need to support multiple kinds of caches simulataneously, then you can do the injection instead of using a static class. No need to go all interface crazy until you actually need it.

Again this is just a starting point. My point is, you should be able to find a way to make everyone happy.

Answer 3

Your class is clearly superior because you can inject a mock cache provider.

Although the HttpContext.Current null check will stop an exception being thrown when the repository or classes which use it are tested. So you aren't offering any concrete benefit.

I wouldn't attempt to justify your code, or respond to the refutations. You've spent some time re-factoring, your changes are clearly in tune with accepted practices, re-factoring is a good thing. What's to complain about?

Given that you have had to justify this and the nature of the refutations I think its clear that really there isn't any time allocated for refactoring. This is the classic 'make it fast' AND 'make it good' AND 'don't spend more money' tricotomy.