ASP.Net caching manager

Question

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?

• sDima: You can view updated version of code here: coderemarks.com/review/LIgmRrlPmwJ2PEuk

Answer 1

/// <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;
}

• sDima: Thank you for very useful style guidelines. But I dont 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 its 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.
• Mathieu Guindon: 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.
• sDima: 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.
• Mathieu Guindon: 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.

Answer 2

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

• Mathieu Guindon: +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 ;)
• Jeroen Vannevel: 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)
• Mathieu Guindon: 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.
• Jeroen Vannevel: 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).
• Mathieu Guindon: 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).

Answer 3

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 (!TryGet<T>(key, out value))
 {
 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

• sDima: Thanks for your reply. I think we can write your function a bit simpler. Please, view my version of it. coderemarks.com/review/LIgmRrlPmwJ2PEuk
• Sergey Litvinov: Yeah, with TryGet<T> it looks better!