current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

I'm willing to simplify the use of the ASP.Net cache. I wrote this helper class :

public static class CacheHelper
{
    public static T GetCached<T>(string key, Func<T> initializer, DateTime absoluteExpiration)
    {
        return GetCached(key, initializer, Cache.NoSlidingExpiration, absoluteExpiration);
    }
    public static T GetCached<T>(string key, Func<T> initializer, TimeSpan slidingExpiration)
    {
        return GetCached(key, initializer, slidingExpiration, Cache.NoAbsoluteExpiration);
    }
    public static T GetCached<T>(string key, Func<T> initializer, TimeSpan slidingExpiration, DateTime absoluteExpiration)
    {
        var httpContext = HttpContext.Current;

        if (httpContext != null)
        {
            var obj = httpContext.Cache[key];
            if (obj == null)
            {
                obj = initializer();
                httpContext.Cache.Add(key, obj, null, absoluteExpiration, slidingExpiration, System.Web.Caching.CacheItemPriority.Default, null);
            }
            return (T)obj;
        }
        else
        {

            return initializer(); // no available cache
        }
    }
}

The idea is to let the developer write its method as usual, but wrapping it like this:

public MyClass GetSomeCachedData(){
            return CacheHelper.GetCached(
                "GetSomeCachedData, () =>
                {

                        return ComputeExpensiveResult();
                }
                }, TimeSpan.FromMinutes(1), Cache.NoAbsoluteExpiration);
}

Do you see any improvement / issue?

PS/ I'm sticked to .Net 3.5

share|improve this question
    
You could simplify your lambda to () => ComputeExpensiveResult(), or even just ComputeExpensiveResult. Also, GetSomeCachedData() should probably be a property. – svick May 7 '13 at 16:55
1  
@svick: actually the content of the lambda is most of time the body of the method, without the cache. I use this simplified syntax to focus on the helper method. That said, I don't agree with changing the method to a property. By definition a property should be inexpensive. In my case, this can be. Moreover, the typical usage of my helper method is to cache the result of a restful web service and the method is actually an operation of the service. – Steve B May 7 '13 at 17:17
up vote 1 down vote

Several suggestions:

  1. You could use MemoryCache class as of .NET 4, which would free you from System.Web assembly.
  2. You should use lock in order to put the value to cache.
  3. You should take care of value types objects

So, here is the code:

public static T GetCached<T>(string key, Func<T> initializer, TimeSpan slidingExpiration, DateTime absoluteExpiration)
{
    var httpContext = HttpContext.Current;

    if (httpContext != null)
    {
        key = string.Intern(key); 
        lock(key) // locking on interned key
        {
            var obj = httpContext.Cache[key];
            if (obj == null)
            {
                obj = initializer();
                httpContext.Cache.Add(key, obj, null, absoluteExpiration, slidingExpiration, System.Web.Caching.CacheItemPriority.Default, null);
            }
            // taking care of value types
            if (obj == null && (typeof (T)).IsValueType)
            {
                return default(T);
            }
            return (T)obj;
        }
    }
    else
    {
        return initializer(); // no available cache
    }
}
share|improve this answer
    
as I said, I'm sticked to the version 3.5 of the framework. That said, I think your lock is useless as the key is a string, and make the lock useless as the instance of the string will always be different, isn't it? – Steve B May 21 '13 at 15:59
    
Every time the method would called with same key value, string.Intern(key) would return reference to same string, so lock should work as needed. – v0id May 21 '13 at 21:16
    
I've learn something today, thanks. – Steve B May 22 '13 at 6:54

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.