C# cache controller (Followup)

Question

This is a followup to the question:
C# cache controller

Suggestions taken onboard from previous feedback, and refactoring to shorten the code in the calls to the cache controller by allowing queries to be passed as parameters.

Problem

Remove repeating cache code throughout code by creating a new cache controller class.

Cache Controller Class

public class LocalCacheController
{
    private static readonly ConcurrentDictionary<string, object> Locks = new ConcurrentDictionary<string, object>();

    /// <summary>
    /// Return cache object
    /// </summary>
    private static Cache Cache => HttpContext.Current?.Cache ?? HttpRuntime.Cache;

    /// <summary>
    /// Return key of an object
    /// </summary>
    private static string GetCacheKey<T>(int recordID)
    {
        return EntityObj.GetCacheKey<T>(recordID);
    }

    /// <summary>
    /// Given a set of record ID's, return the uncached
    /// </summary>
    public static List<int> GetUncachedRecordIDs<T>(List<int> recordIDsToQuery)
    {
        return recordIDsToQuery
            .Where(c => !IsCached(GetCacheKey<T>(c)))
            .ToList();
    }

    /// <summary>
    /// Bulk save objects in cache if they don't exist.
    /// </summary>
    public static void AddRange<T1, T2>(IQueryable<T1> uncached)
    {
        foreach (dynamic rec in uncached)
        {
            int recordID = rec.ID;
            LocalCacheController.Add<T1, T2>(recordID, rec);
        }
    }

    /// <summary>
    /// Set value in cache
    /// </summary>
    public static T2 Add<T1, T2>(int recordID, T1 getCachableObject)
    {
        Common.TestTypeDerivedFromDatabase<T1>();

        var key = GetCacheKey<T2>(recordID);

        var obj = Cache[key];
        if (obj != null) return (T2)obj;

        try
        {
            Locks.TryAdd(key, new object());
            lock (Locks[key])
            {
                obj = Cache[key];
                if (obj == null)
                {
                    obj = Activator.CreateInstance(typeof(T2), getCachableObject);
                    SetLocalCacheValue(obj, false, key);
                }
            }
        }
        finally
        {
            object tR;
            Locks.TryRemove(key, out tR);
        }

        return (T2)obj;
    }

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

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

    /// <summary>
    /// Is there an object at key
    /// </summary>
    private static bool IsCached(string key)
    {
        return Cache[key] != null;
    }

    /// <summary>
    /// Get object from cache
    /// </summary>
    public static T GetFromCache<T>(int recordID)
    {
        var key = GetCacheKey<T>(recordID);
        var obj = Cache[key];
        if (obj == null) return default(T);
        return (T)obj;
    }
}

Uncached query

/// <summary>
/// Return this object when querying which record ID's are currently uncached
/// for a specific type.
/// </summary>
public struct UncachedQueryResult<T>
{
    /// <summary>
    /// The returned uncached record IDs
    /// </summary>
    public List<int> UncachedRecordIDs { get; set; }

    /// <summary>
    /// Is every record ID passed in cached?
    /// </summary>
    public bool AllCached
    {
        get { return UncachedRecordIDs.Count == 0; }
    }

    private UncachedQueryResult(List<int> queryRecordIDs)
    {
        UncachedRecordIDs = LocalCacheController.GetUncachedRecordIDs<T>(queryRecordIDs);
    }

    /// <summary>
    /// Perform a new query on a set of record IDs
    /// </summary>
    public static UncachedQueryResult<T> Get(List<int> queryRecordIDs)
    {
        return new UncachedQueryResult<T>(queryRecordIDs);
    }
}

Example usage

Retrieve a single private message by ID

/// <summary>
/// Return a private message by it's ID
/// </summary>
public static PrivateMessage GetPrivateMessage(int messageID)
{
    var pm = LocalCacheController.GetFromCache<PrivateMessage>(messageID);
    if (pm != null) return pm;

    using (var db = new DBContext())
    {
        return LocalCacheController
            .Add<DB.PrivateMessage, PrivateMessage>(
                messageID,
                CompiledQueries.GetPMByID(db, messageID)
            );
    }
}

Cache any uncached private messages by ID

/// <summary>
/// Given list of message ID's, cache any uncached
/// </summary>
public static void CacheUncachedMessageIDs(List<int> messageIDs)
{
    var q = UncachedQueryResult<PrivateMessage>.Get(messageIDs);
    if (q.AllCached) return;

    using (var db = new DBContext())
    {
        LocalCacheController.AddRange<DB.PrivateMessage, PrivateMessage>(
            db.PrivateMessages.Where(c => q.UncachedRecordIDs.Contains(c.ID))
        );
    }
}

Answer 1

As was brought up in the comments, I find static classes with state to almost never be what you want. If the code that uses your class wants to create a single instance and store it in a static variable, they can do that. This particular class does itself reference an existing static cache, but I see that as a reason to refactor that aspect (to e.g. accept it as a constructor parameter) rather than a reason to make this class static.

Maybe it's just me, but LocalCacheController doesn't really mean anything to me. Local cache seems redundent in this context, and I'm not sure what it's controlling. As for the API, the public functions are AddRange, Add, GetFromCache, and GetUncachedRecordIDs. The last one seems not like the others, and the implementation shows that it could be implemented elsewhere - that is, there doesn't seem to be much of a technical reason for it to be part of this class, either.

The class seems to be doing two things:

1. Wrapping an existing cache by providing a more useful API and consistently managing the cache policy and

2. providing key translation and an additional uncached query. I wonder if it wouldn't be better as two smaller classes. As for the API, having GetFromCache be generic doesn't seem to make life easier for you. Since the generic type is just used as the return, you have to specify it in every call, in which case you might as well return object and have the caller perform the cast. Contrast this with the generic collections, where the generic is part of the instance rather than method and so the type doesn't have to be applied to the Get functions. You could do the same thing here, moving the generic parameter to the type, but it really only works if you also make the class non-static.

For the Add function, it's not clear from the names or the comments what getCachableObject is supposed to be. The name makes it sound like a function, but the convention in that case is to type it as a Func. If there's a reason not to accept a factory function I don't see what it is - it's strictly more flexible than using Activator, and it allows better typechecking. Maybe there's more going on here that explains the decision - I don't know enough about CompiledQueries.GetPMByID to say. I do see that your previous version used Func, so I wonder why you changed it. On that topic, GetPrivateMessage does look like it contains a pattern that you'll repeat for each type in your cache, so it might be nice to make that pattern generic and move it to a place that makes it reusable.