7

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?

  • KISS/YAGNI – user7519 May 14 '15 at 15:51
  • 3
    "We're not going to use a different caching technology in the foreseeable future." Are you ever going to use this repository on a thread other than the request thread? If so, have fun switching from HttpContext.Current.Cache to HttpRuntime.Cache everywhere. This isn't just about changing implementation, it's about being DRY with the information of how the cache is accessed. – Ben Aaronson May 14 '15 at 17:05
  • 1
    As BenAaronson pointed out, a pragmatist will see that HttpContext.Current imposes a thread-local requirement on the code, which may impede future refactoring or design patterns. A pragmatist is also well-informed and know that other pragmatists had been burned by this before, so that this threat is credible (not something remote at all), and will heed the advice. Therefore, a smart YAGNI-ist will go the way of pragmatist and take the same advice too. – rwong May 14 '15 at 20:44
9

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.

  • Thanks for this answer. The reason why I was changing that implementation is I was using it in a new web project that has a lot of unit tests and an IoC container. The main issue is that the project in which this library lies does not have any consistency I can discern. – Matthew May 15 '15 at 13:30
1

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.

  • I actually do have an extension method on my ICacheProvider that does something similar. – Matthew May 15 '15 at 13:27
0

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.

  • 3
    his changes are clearly not in tune with accepted practices at his company. As a result, he's the one out of step with everyone else and thus wrong. Fixing the practices there is a constructive move (and then modify all the code), rewriting code to suit your preferences of what is 'best' is simply disruptive, and often pointless. – gbjbaanb May 14 '15 at 16:34
  • 1
    @gbjbaanb Eh, I think you're making assumptions about what kind of company he's at. It's pretty common for there not to be clear company-wide consensus on practices, and for things like this to be worked out on a pair/small group level. – Ben Aaronson May 14 '15 at 16:59
  • 1
    @BenAaronson I went with refutations #4 and #5 - they suggest the others in the team don't see things the same way. We have to work with what we've got here! – gbjbaanb May 14 '15 at 18:14
  • Although testability is important, isn't is a slight biased approach to deem a bit of code superior just because it improves testing? – Alex May 14 '15 at 22:34
  • The key quote for me is "assuming we have time allocated for cleaning up code" I don't think anyone would argue that the re-factored code is not better from a CS perspective. So it seems to me the subtext is, actually, there is no time for re-factoring after all. – Ewan May 15 '15 at 7:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.