Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

We've started implementing some caching code in our MVC project. We've derived from .Net's ObjectCache to store it in a SQL database.

What we do right now is in each method we want to cache we have code like this:

public StatusCounts GetCounts()
{
   var cache = new SqlCache<StatusCounts>("mykey");
   if(cache.HasCachedData())
      return cache.GetCachedData();

   var data = StatusRepository.GetCounts();

   cache.SetData(data, DateTime.Now.AddHours(2));
}

As far as caching goes, it works. But my concern is it seems like now GetCounts has multiple responsibilities. It's no longer just calculating counts, it's getting them from a cache (if they exist) and saving them to a cache before returning.

Is there a cleaner way to do this?

share|improve this question
    
Wouldn't you want to push the cache checking responsibility back up to the StatusRepository? –  scott-pascoe Feb 19 '13 at 20:39
    
You're right, it should be further down the repository. The code above was simplified. There might be other things going on between just pulling straight from the repo. But in most cases, it should just be in the repo. –  taylonr Feb 20 '13 at 13:54

3 Answers 3

up vote 7 down vote accepted

You can separate the cache initialization from returning the value, and implement the cache initialization in a generic way:

public StatusCounts GetCounts()
{
   var cache = GetInitializedCache<StatusCounts>("mykey",StatusRepository.GetCounts,2);
   return cache.GetCachedData();
}

private SqlCache<T> GetInitializedCache<T>(string key,Function<T> getData,double hours)
{
   var cache = new SqlCache<T>(key);
   if(!cache.HasCachedData())
   {
      T data = getData();
      cache.SetData(data, DateTime.Now.AddHours(hours));
   }
   return cache;
}

That will separate the concerns of caching and calculating the values as well as giving you a reusable function. Of course, the method body of GetCounts is still aware of the caching mechanics, and if you want to avoid that too, @Peri's answer maybe the right approach (for the price of having to introduce a non-trivial third-party component).

share|improve this answer
    
I really don't like this, this code is far more confusing and nonobvious in intention. I would much rather work with a codebase similar to the op's "dumb" code. –  user7550 Feb 19 '13 at 18:13
1  
@ChrisMarisic: feel free to suggest better method names to make the intentions more clear. But don't forget the OP wrote he has similar code in lots of methods - so the original code contains lots of duplicated code snippets. My suggestion adresses this, now one can reuse that cache initialization in many places, making the code much more DRY. –  Doc Brown Feb 19 '13 at 20:37

I would try to implement it using AOP. You could try Castle.Windsor's Interceptors or mechanism other IoC container have. That way you implement caching in one place only (Interceptor) and there is no sign of cache in your method.

http://docs.castleproject.org/Default.aspx?Page=Interceptors&NS=Windsor&AspxAutoDetectCookieSupport=1

share|improve this answer
1  
I feel this would lead to more complex code than explicitely indicating that getCounts actually handle caching. With AOP, getCounts still manage caching (even indirectly), thus is subject to all caching errors/exceptions/etc. Looks a little like "hide it under the carpet so no one sees it and it looks like it's clean" –  Clement Herreman Feb 19 '13 at 15:05
    
I strongly disagree. Putting same code in every method is bad. Look at that method above. 4 lines of code for caching and one line of real business code. Same as when people do Log.Trace("Entering method xxx with parameters x, y, z"), Log.Trace("Leaving method XXX with result YYY). That's not what you want to do in your business logic code. That's infrastructure! –  Peri Feb 19 '13 at 15:07
    
IMO the difference is that log doesn't have any effect to the returned/processed values. Those lines doesn't have to be copy/pasted, they can be factorized somewhere (see Doc Brown answer). What I try to emphasis here is that when looking at code, I find that explicitely seeing the cache layer is better than wondering if some AOP is doing something to my value/param/returns –  Clement Herreman Feb 19 '13 at 15:18
    
@ClementHerreman I don't see any way around getCounts() dealing with caching exception, if you want to keep the same semantics. –  svick Feb 19 '13 at 15:43
    
@ClementHerman: You can decorate methods with some attribute to enable caching. And in your interceptor check if attribute is present I guess. But that could slow things down. –  Peri Feb 19 '13 at 15:51

I built a little tool for this some time ago, https://code.google.com/p/mbcache, that we use with success in our shop. It uses AOP the way @Peri describes behind the scenes.

No matter using this or another tool, I would defiantly favor AOP for this. As you write yourself, you shouldn't mix responsibilities within your classes (extract caching logic to a seperate method don't matter IMHO - it just moves the dependency instead of getting rid of it).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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