Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a Cache Helper Class.

using System;
using System.Web;

public static class CacheHelper
{
    /// <summary>
    /// Insert value into the cache using
    /// appropriate name/value pairs
    /// </summary>
    /// <typeparam name="T">Type of cached item</typeparam>
    /// <param name="o">Item to be cached</param>
    /// <param name="key">Name of item</param>
    public static void Add<T>(T o, string key) 
    {
        // NOTE: Apply expiration parameters as you see fit.
        // I typically pull from configuration file.

        // In this example, I want an absolute
        // timeout so changes will always be reflected
        // at that time. Hence, the NoSlidingExpiration.
        HttpContext.Current.Cache.Insert(
            key,
            o,
            null,
            DateTime.Now.AddMinutes(1440),
            System.Web.Caching.Cache.NoSlidingExpiration);
    }

    /// <summary>
    /// Remove item from cache
    /// </summary>
    /// <param name="key">Name of cached item</param>
    public static void Clear(string key)
    {
        HttpContext.Current.Cache.Remove(key);
    }

    /// <summary>
    /// Check for item in cache
    /// </summary>
    /// <param name="key">Name of cached item</param>
    /// <returns></returns>
    public static bool Exists(string key)
    {
        return HttpContext.Current.Cache[key] != null;
    }

    /// <summary>
    /// Retrieve cached item
    /// </summary>
    /// <typeparam name="T">Type of cached item</typeparam>
    /// <param name="key">Name of cached item</param>
    /// <param name="value">Cached value. Default(T) if 
    /// item doesn't exist.</param>
    /// <returns>Cached item as type</returns>
    public static bool Get<T>(string key, out T value) 
    {
        try
        {
            if (!Exists(key))
            {
                value = default(T);
                return false;
            }

            value =  (T) HttpContext.Current.Cache[key];
        }
        catch
        {
            value = default(T);
            return false;
        }

        return true;
    }
}

and usage:

string key = "EmployeeList";
List<Employee> employees;

if (!CacheHelper.Get(key, out employees))
{
    employees = DataAccess.GetEmployeeList();
    CacheHelper.Add(employees, key);
    Message.Text =
        "Employees not found but retrieved and added to cache for next lookup.";
}
else
{
    Message.Text = "Employees pulled from cache.";
}

Do you see any improvement / issue?

share|improve this question
    
You could just put the updated code links as comments under the question. It's noisy to have them within the question. –  Jamal May 14 at 16:07
    
You can view updated version of code here: coderemarks.com/review/LIgmRrlPmwJ2PEuk –  sDima May 14 at 16:16
add comment

3 Answers 3

/// <summary>
/// Insert value into the cache using
/// appropriate name/value pairs
/// </summary>
/// <typeparam name="T">Type of cached item</typeparam>
/// <param name="o">Item to be cached</param>
/// <param name="key">Name of item</param>
public static void Add<T>(T o, string key)

I think this signature would be more useful like this:

/// <summary>
/// Insert value into the cache using
/// appropriate name/value pairs.
/// </summary>
/// <typeparam name="T">Type of cached item (inferred from usage).</typeparam>
/// <param name="key">A string used to uniquely identify the added value.</param>
/// <param name="value">Item/value to be cached.</param>
public static void Add<T>(string key, T value)

This is misleading:

/// <summary>
/// Remove item from cache
/// </summary>
/// <param name="key">Name of cached item</param>
public static void Clear(string key)
{
    HttpContext.Current.Cache.Remove(key);
}

Clear is usually a parameterless method that clears a container's contents. This method should be named Remove.


/// <summary>
/// Check for item in cache
/// </summary>
/// <param name="key">Name of cached item</param>
/// <returns></returns>
public static bool Exists(string key)
{
    return HttpContext.Current.Cache[key] != null;
}

"Check for item in cache" doesn't really say what's going on. I realize it's trivially inferred from the method's name, but "Checks for existence of specified key in cache." would be totally unambiguous.


public static bool Get<T>(string key, out T value) 

I don't like the implementation of that method very much, there are too many exit points, some being redundant. Consider something like this:

public static bool TryGet<T>(string key, out T value) 
{
    bool result;

    try
    {
        if (Exists(key))
        {
            value =  (T)HttpContext.Current.Cache[key];
            result = true;
        }
    }
    catch(InvalidCastException)
    {
        value = default(T);
    }

    return result;
}
share|improve this answer
    
Thank you for very useful style guidelines. But I don`t understand about using HttpContext.Current.Cache.Items. HttpContext.Current.Cache does not contain a definition for Items. If you mean HttpContext.Current.Items this is not the same. Items is per request, so it`s only available for that given user for that given HTTP request. Cache is stored in memory for a persistent period of time, and it not dependent on the specific user. So cache can be shared across multiple users across multiple requests, but Items is per user per request. –  sDima May 13 at 10:49
    
I meant the indexer - Cache[n] should be the same as Cache.Items[n]. I'll read up on that, I don't use an HttpContext very often. –  Mat's Mug May 13 at 11:00
    
Yes, I know it, but I can't use HttpContext.Current.Cache.Contains(key), because in HttpContext.Current.Cache has no definition of Contains() or something like this. –  sDima May 13 at 11:14
    
Looks like I was looking at the wrong MSDN docs.. the indexer uses the Item property and indeed there's no Items property. Sorry for the mix-up, edited my answer. –  Mat's Mug May 14 at 16:06
add comment
try
{
    if (!Exists(key))
    {
        value = default(T);
        return false;
    }

    value =  (T) HttpContext.Current.Cache[key];
} catch {
    value = default(T);
    return false;
}

return true;

This contains some duplicate logic. We can rewrite it like this:

if (Exists(key))
{
    value = (T) HttpContext.Current.Cache[key];
    return true;
}

value = default(T);
return false;

Advantages:

  • No negative condition check
  • No duplication of the default(T)
  • No expensive try-catch (all) block
share|improve this answer
    
+1 I like this simple & neat implementation. However try/catch is only "expensive" if the stack gets unwinded, i.e. if an actual exception is thrown... and there's a possible InvalidCastException if the cached object can't be cast to T - granted it's a misuse, but still a possible execution path. The class can't assume its client knows what it's doing ;) –  Mat's Mug May 12 at 16:47
2  
Good point about the InvalidCastException. If you want to explicitly catch it then you can add it but it would give the impression that a key is not present and thus would hide the problem in its implementation. Since it is impossible in the first place to use multiple equal keys, I think throwing the exception is more appropriate (it also helps with debugging) –  Jeroen Vannevel May 12 at 16:55
    
not necessarily: the method infers the type of T from the value parameter, which puts the entire responsibility of knowing the type of a cached item, on the caller, regardless of whether or not the specified key exists, hence my distrust for the [assumed-to-be-]safe cast. –  Mat's Mug May 12 at 17:04
1  
The alternative by catching the exception would be to return null so the user would have to explicitly call Exists before Get and make some weird flow to account for the possibility that the key might exist but he's trying to retrieve it with the different type (which doesn't show up any different from not having the key in the first place). Since there shouldn't be any surprise as to what type a key is, I don't think it adds any value to hide the exception (but it will show up as an immediate problem if you used the wrong type). –  Jeroen Vannevel May 12 at 17:10
    
Damn, blinded by the trees again - you're right, it's best to just let the InvalidCastException bubble up, it's the most meaningful result a caller can get (vs a default(T) value and a false result for a key that does exist). –  Mat's Mug May 12 at 17:13
add comment

I would suggest one more solution how to make usage a bit easier. You can add this method into CacheHelper

public static T GetOrAdd<T>(string key, Func<T> getter)
{
    T value;
    if (Exists(key))
    {
        Get<T>(key, out value);
    }
    else
    {
        value = getter();
        Add(value, key);
    }

    return value;
}

In that case usage can be a bit simplier, but in that case you won't know is item existed in cache or not

string key = "EmployeeList";
List<Employee> employees = CacheHelper.GetOrAdd(key, () =>  DataAccess.GetEmployeeList());
// or even like this if method matches signature
List<Employee> employees = CacheHelper.GetOrAdd(key, DataAccess.GetEmployeeList);

If item doesn't exist, then it will call lambda, and will add it to the cache

share|improve this answer
    
Thanks for your reply. I think we can write your function a bit simpler. Please, view my version of it. coderemarks.com/review/LIgmRrlPmwJ2PEuk –  sDima May 14 at 8:36
    
Yeah, with TryGet<T> it looks better! –  Sergey Litvinov May 14 at 8:48
add comment

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.