6

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?

Improve this question
2
  • Wouldn't you want to push the cache checking responsibility back up to the StatusRepository? Commented Feb 19, 2013 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. Commented Feb 20, 2013 at 13:54

3 Answers 3

Reset to default
7

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).

Improve this answer
2
  • 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. Commented Feb 19, 2013 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. Commented Feb 19, 2013 at 20:37
4

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

Improve this answer
10
  • 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" Commented Feb 19, 2013 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! Commented Feb 19, 2013 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 Commented Feb 19, 2013 at 15:18
  • @ClementHerreman I don't see any way around getCounts() dealing with caching exception, if you want to keep the same semantics. Commented Feb 19, 2013 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. Commented Feb 19, 2013 at 15:51
0

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).

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.