3
\$\begingroup\$

Problem

Currently my project utilises local caching wherever possible. There is a lot of similar looking code throughout that takes the form:

private static readonly ConcurrentDictionary<int, object> GetPMLocks = new ConcurrentDictionary<int, object>();
/// <summary>
/// Retrieve a PM by it's unique ID
/// </summary>
public static PrivateMessage GetPrivateMessage(int messageID)
{
    var cacheIndex = GetPMCacheIndex(messageID);
    var cache = GetCache();
    if (cache[cacheIndex == null])
    {
        try
        {
            GetPMLocks.TryAdd(messageID, new object());
            lock (CacheCommentLocks[messageID])
            {
                if (cache[cacheIndex] == null)
                {
                    using (var db = new DBContext())
                    {
                        var q = CompiledQueries.GetPMByID(db, messageID);
                        var pm = new PrivateMessage(q);

                        cache.Add(cacheIndex, pm, null,
                        Cache.NoAbsoluteExpiration,
                        Cache.NoSlidingExpiration,
                        CacheItemPriority.Normal,
                        null);
                    }
                }
            }
        }
        finally
        {
            object tR;
            GetPMLocks.TryRemove(messageID, out tR);
        }
    }
    return (PrivateMessage) cache[cacheIndex];
}

Solution

I wish to refactor this to be more concise, and to route every cache request through a central controller so it's easy to fix bugs/modify & measure the caching process and performance. So I've created a new LocalCacheController class:

public class LocalCacheController
{
    private Cache CacheObj_ { get; set; }
    private Cache CacheObj {
        get
        {
            if (CacheObj_ == null) CacheObj_ = GetCache();
            return CacheObj_;
        }
    }

    /// <summary>
    /// Cache index this cacher is for
    /// </summary>
    private string ForCacheIndex { get; }

    public LocalCacheController(string cacheIndex, Cache cacheObj = null)
    {
        CacheObj_ = cacheObj;
        ForCacheIndex = cacheIndex;
    }

    /// <summary>
    /// Bulk save objects in cache if they don't exist.  Each tuple item needs an object to be saved, and a cache index
    /// </summary>
    public static void SetIfNotExists<T>(List<Tuple<T, string>> items)
    {
        // Nothing to do
        if (!items.Any()) return;

        var cache = GetCache();
        foreach (var item in items)
        {
            var controller = new LocalCacheController(item.Item2, cache);
            controller.SetIfNotExists(item.Item1);
        }
    }

    private static readonly ConcurrentDictionary<string, object> Locks = new ConcurrentDictionary<string, object>();

    /// <summary>
    /// Set a value if it doesn't exist
    /// </summary>
    /// <returns>Value in cache</returns>
    private T SetIfNotExists<T>(T obj)
    {
        SetLocalCacheValue(obj, false);
        return (T)CacheObj[ForCacheIndex];
    }

    /// <summary>
    /// Set value in cache.  If cache is empty, gets object from passed function
    /// </summary>
    /// <param name="getCachableObjectFunction">Function that returns the values to be cached</param>
    /// <returns>Value in cache either that already existed there or was just set</returns>
    public T SetIfNotExists<T>(Func<T> getCachableObjectFunction)
    {
        if (!IsCached(ForCacheIndex, CacheObj))
        {
            // We want to lock here, as we only wanted to ever call the 
            // passed function once as it can be expensive
            try
            {
                Locks.TryAdd(ForCacheIndex, new object());
                lock (Locks[ForCacheIndex])
                {
                    if (!IsCached(ForCacheIndex, CacheObj))
                    {
                        var obj = getCachableObjectFunction();
                        SetLocalCacheValue(obj, false);
                    }
                }
            }
            finally
            {
                object tR;
                Locks.TryRemove(ForCacheIndex, out tR);
            }
        }
        return (T)CacheObj[ForCacheIndex];
    }

    /// <summary>
    /// Sets a value in a cache regardless if it exists or not
    /// </summary>
    /// <returns>The object cached, or the object already in cache</returns>
    public T SetValue<T>(object objectToCache)
    {
        SetLocalCacheValue(objectToCache, true);
        return (T)CacheObj[ForCacheIndex];
    }

    /// <summary>
    /// Actually sets the value in the cache
    /// </summary>
    private void SetLocalCacheValue(object objectToCache, bool overwrite)
    {
        const CacheItemPriority priority = CacheItemPriority.Normal;
        var slidingExpiry = Cache.NoSlidingExpiration;
        var absoluteExpiry = Cache.NoAbsoluteExpiration;

        if (overwrite)
        {
            CacheObj.Insert(ForCacheIndex, objectToCache, null, absoluteExpiry, slidingExpiry, priority, null);
        }
        else
        {
            CacheObj.Add(ForCacheIndex, objectToCache, null, absoluteExpiry, slidingExpiry, priority, null);
        }
    }

    /// <summary>
    /// Is there an object in cache index
    /// </summary>
    public static bool IsCached(string cacheIndex, Cache cache = null)
    {
        if (cache == null) cache = GetCache();
        return cache[cacheIndex] != null;
    }

    /// <summary>
    /// Get cached bytes from local cache
    /// </summary>
    /// <param name="cacheIndex">Unique index to lookup</param>
    /// <param name="cache">Cache object if known</param>
    /// <returns>Null if cache index doesn't exist</returns>
    public static object GetFromLocalCache(string cacheIndex, Cache cache = null)
    {
        if (cache == null) cache = GetCache();
        if (cache[cacheIndex] == null) return null;
        return cache[cacheIndex];
    }

    /// <summary>
    /// Return cache object
    /// </summary>
    private static Cache GetCache()
    {
        return (HttpContext.Current == null)
                ? HttpRuntime.Cache
                : HttpContext.Current.Cache;
    }
}

Example Usages with Controller

1: Caching a single private message

This would replace the original "problem" code shown at the start of this post:

public static PrivateMessage GetPrivateMessage(int messageID)
{
    var cacheObj = new LocalCacheController(GetPMCacheIndex(messageID));
    var pm = cacheObj.SetIfNotExists(() => GetPM(messageID));
    return pm;
}
private static PrivateMessage GetPM(int messageID)
{
    using (var db = new DBContext())
    {
        var q = CompiledQueries.GetPMByID(db, messageID);
        if (q == null) return null;
        return new PrivateMessage(q);
    }
}

2: Given a list of private message ID's, cache the uncached ones

public static void CacheUncachedMessageIDs(List<int> messageIDs)
{
    var uncached = messageIDs.Where(c => !LocalCacheController.IsCached(GetPMCacheIndex(c))).ToList();
    LocalCacheController.SetIfNotExists(GetPMs(uncached));
}
private static List<Tuple<PrivateMessage, string>> GetPMs(ICollection<int> messageIDs)
{
    var r = new List<Tuple<PrivateMessage, string>>();
    if (!messageIDs.Any()) return r;

    using (var db = new DBContext())
    {
        var q = db.PrivateMessages.Where(c => messageIDs.Contains(c.ID));
        foreach (var rec in q)
        {
            r.Add(new Tuple<PrivateMessage, string>(new PrivateMessage(rec), GetCacheIndex(rec.ID)));
        }
    }

    return r;
}

I believe I've achieved the objective of making the code more readable and concise. As this code is being relied on heavily throughout the project I would appreciate any input!

\$\endgroup\$
2
\$\begingroup\$

You have a lots of repetitions there:

private Cache CacheObj_ { get; set; }
private Cache CacheObj
{
  get
  {
      if (CacheObj_ == null) CacheObj_ = GetCache();
      return CacheObj_;
  }
}

and

private static Cache GetCache()
{
  return (HttpContext.Current == null)
          ? HttpRuntime.Cache
          : HttpContext.Current.Cache;
}

This can be all shortend to just:

private Cache Cache => HttpContext.Current?.Cache ?? HttpRuntime.Cache;

public LocalCacheController(string cacheIndex, Cache cacheObj = null)

You don't need to pass the Cache object around. It's actaully static so if you make the Cache propertey static too then you're done.

private satatic Cache Cache => ...

public static void SetIfNotExists<T>(List<Tuple<T, string>> items)

As the use of the cache I don't want to know if something already exists or not. I just want to add something to the cache and it should handle the exists/does not exist questions internally.

This should be just AddRange.


private string ForCacheIndex { get; }

This really needs a better name. It's netiher an index nor a for. I suggest something like CacheKey as this looks like one.


public static object GetFromLocalCache(string cacheIndex, Cache cache = null)

As a user I don't really want to know where the object comes from (unless there are more sources and here aren't any other). A simple GetValue or GetObject would be enough. cacheIndex should be key.

\$\endgroup\$
  • 1
    \$\begingroup\$ Good comments, thank you! Have implemented all suggestions (and done some further refactoring) and it's much more concise now. \$\endgroup\$ – Tom Gullen Oct 31 '16 at 12:45

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.