current community

your communities

more stack exchange communities

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

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

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);
    }
}
share|improve this question
up vote 4 down vote accepted

  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 you build the key you only need to touch one method rather than all of them.

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

  4. Your 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.

share|improve this answer
    
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 :) – Heidel Ber Gensis Mar 1 '14 at 8:46
    
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). – svick Mar 1 '14 at 12:31
    
@svick but the CacheMemory itself is threadsafe, wouldn't that make it thread safe? – Heidel Ber Gensis Mar 1 '14 at 12:33
    
@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. – svick Mar 1 '14 at 12:35
up vote 2 down vote

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

share|improve this answer

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.