4
\$\begingroup\$

I want to have a caching class to cache different types. I want each type to be cached in a different MemoryCache but in a generic way.

Am I doing it right?

internal static class RecordsCache
{
    private static Dictionary<string, ObjectCache> cacheStore;
    static private CacheItemPolicy policy = null;

    static RecordsCache()
    {
        cacheStore = new Dictionary<string, ObjectCache>();

        ObjectCache activitiesCache = new MemoryCache(typeof(Activity).ToString());
        ObjectCache lettersCache = new MemoryCache(typeof(Letter).ToString());
        ObjectCache contactssCache = new MemoryCache(typeof(Contact).ToString());

        cacheStore.Add(typeof(Activity).ToString(), activitiesCache);
        cacheStore.Add(typeof(Letter).ToString(), lettersCache );
        cacheStore.Add(typeof(Contact).ToString(), contactssCache );

        policy = new CacheItemPolicy();
        policy.Priority = CacheItemPriority.Default;
        policy.AbsoluteExpiration = DateTimeOffset.Now.AddHours(12);
    }

    public static void Set<T>(string userID, int year, List<T> records)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();
        cache.Set(key, records, policy);
    }

    public static bool TryGet<T>(string userID, int year, out List<T> records)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();

        records = cache[key] as List<T>;
        return records != null;
    }

    public static void Remove<T>(string userID, int year)
    {
        var cache = cacheStore[typeof(T).ToString()];
        string key = userID + "-" + year.ToString();
        cache.Remove(key);
    }
}
\$\endgroup\$
7
\$\begingroup\$
  1. Instead of having a cache

    private static Dictionary<string, ObjectCache> cacheStore;
    

    you could have one keyed of the types:

    private static Dictionary<Type, ObjectCache> cacheStore;
    

    This means you don't need to call typeof(T).ToString() everywhere.

  2. You duplicate the code of building the key in all 3 methods - it should be extracted into a private BuildKey(string userId, int year) method. This means if you need to change how the key is built you only need to touch one method rather than all of them.

  3. I would provide a Register method which accepts a type for which to create a new cache rather than hard-coding it in.

  4. The cache policy is also not configurable - which means you are stuck with it unless you want to change the code.

  5. You really really should think hard about whether a static cache is the right way to go. It will give you trouble in unit testing and it will also make it impossible for multiple classes to have different cache with different policies for example.

\$\endgroup\$
  • \$\begingroup\$ Thanks, very valuable tips. I am no programmer in real life, it is just a hobby. I never had a programming courses or so. So your tips are like gold to me :) \$\endgroup\$ – Nean Der Thal Mar 1 '14 at 8:46
  • \$\begingroup\$ One more issue with keeping the class static is that it's not thread-safe. And static classes that are not thread-safe are almost unusable in multi-threaded programs (and should be very clearly documented as such). \$\endgroup\$ – svick Mar 1 '14 at 12:31
  • \$\begingroup\$ @svick but the CacheMemory itself is threadsafe, wouldn't that make it thread safe? \$\endgroup\$ – Nean Der Thal Mar 1 '14 at 12:33
  • \$\begingroup\$ @MeNoTalk I guess you're right. Dictionary isn't thread-safe, but you only read from it (unless you follow Chris's advice about Register()), so that should be okay. \$\endgroup\$ – svick Mar 1 '14 at 12:35
2
\$\begingroup\$

From my point of view, everything is well coded.

Just some personal preference here:

  1. I would provide 1 more Set<T> method which accepts a func as input instead of List<T>:

    public static void Set(string userId, int year, Func<List<T>> retrieveData) { }
    
  2. I would like to have 1 more method to combine Get and Set together which looks like:

    public List<T> TryGetAndSet(string userId, int year, Func<List<T>> retrieveData)
    {
        //if cache item exist return cacheItem 
        //if cache item does not exist, retrieve data by executing retrieveData
        //  If any result retrieved, set to cache and return as result
    }
    
  3. 1 more item to check whether certain cache item exists

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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