Cache storage data structure

Question

I created this data structure a while back and it's been in use for a while without error (to my knowledge). I'm currently at the end of phase one of the project so I'm doing some clean-up and wanted to see if there were any ways to improve the structure.

CacheStore Structure

public delegate void CacheStoreEventHandler<TKey, TValue>(CacheStore<TKey, TValue> sender, TKey key);
public delegate void CacheStoreExpirationEventHandler<TKey, TValue>(CacheStore<TKey, TValue> sender, TKey key, TimeSpan oldExpiration, TimeSpan newExpiration);

public class CacheStore<TKey, TValue>
{
    private IDictionary<TKey, CacheItem> Cache;

    /// <summary>
    /// An item in the cache will be refreshed on retrieval if it is older than the expiration.
    /// </summary>
    public TimeSpan DefaultExpiration
    {
        get
        {
            return _DefaultExpiration;
        }
        set
        {
            OnExpirationChanged(default(TKey), _DefaultExpiration, value);
            _DefaultExpiration = value;
        }
    }
    private TimeSpan _DefaultExpiration;

    public IEnumerable<TKey> Keys { get { return Cache.Keys; } }


    public CacheStore(TimeSpan expiration)
    {
        _DefaultExpiration = expiration;
        Cache = new Dictionary<TKey, CacheItem>();
    }

    /// <summary>
    /// Executed after adding/changing a cache item.
    /// </summary>
    public event CacheStoreEventHandler<TKey, TValue> Changed;
    protected void OnChanged(TKey key)
    {
        if (Changed != null)
        {
            Changed(this, key);
        }
    }

    /// <summary>
    /// Executed after changing the expiration time of a cache item.
    /// </summary>
    public event CacheStoreExpirationEventHandler<TKey, TValue> ExpirationChanged;
    protected void OnExpirationChanged(TKey key, TimeSpan oldExpiration, TimeSpan newExpiration)
    {
        if (ExpirationChanged != null)
        {
            ExpirationChanged(this, key, oldExpiration, newExpiration);
        }
    }

    /// <summary>
    /// Executed after refreshing the value of a cache item.
    /// </summary>
    public event CacheStoreEventHandler<TKey, TValue> Refreshed;
    protected void OnRefreshed(TKey key)
    {
        if (Refreshed != null)
        {
            Refreshed(this, key);
        }
    }

    /// <summary>
    /// Executed after each time a cache item is gotten from the cache store.
    /// </summary>
    public event CacheStoreEventHandler<TKey, TValue> Retrieved;
    protected void OnRetrieved(TKey key)
    {
        if (Retrieved != null) 
        {
            Retrieved(this, key);
        } 
    }


    /// <summary>
    /// Add a key/value pair to the cache indicating how to retrieve fresh data and when the data was retrieved.
    /// </summary>
    public void Add(TKey key, TValue value, Expression<Func<TValue>> refreshCache, DateTime now)
    {
        Add(key, value, refreshCache, now, DefaultExpiration);
    }

    /// <summary>
    /// Add a key/value pair to the cache indicating how to retrieve fresh data and when the data was retrieved.
    /// </summary>
    public void Add(TKey key, TValue value, Expression<Func<TValue>> refreshCache, DateTime now, TimeSpan expiration)
    {
        var cacheItem = new CacheItem()
        {
            Key = key,
            Value = value,
            RefreshCache = refreshCache.Compile(),
            CachedOn = now,
            Expiration = expiration
        };

        var isNullOrDefault = cacheItem.Value == null || cacheItem.Value.Equals(default(TValue));
        if (isNullOrDefault)
        {
            cacheItem.Value = cacheItem.RefreshCache();
        }

        Cache.Add(key, cacheItem);
        OnChanged(key);
    }


    /// <summary>
    /// Retrieves a value from the cache. If the refresh flag is set or if the
    /// value is older than the expiration the value will be refreshed before returning.
    /// </summary>
    public TValue Get(TKey key, bool isForcingRefresh = false)
    {
        if (!Cache.ContainsKey(key))
        {
            throw new ArgumentOutOfRangeException("Cache doesn't contain supplied key.");
        }

        var now = DateTime.Now;
        var expiration = Cache[key].Expiration;
        if (isForcingRefresh || GetAge(key, now) >= expiration)
        {
            OnRefreshed(key);
            Refresh(key, now);
        }

        OnRetrieved(key);
        return Cache[key].Value;
    }

    /// <summary>
    /// Retrieves the age of the cache item with the indicated key.
    /// Does not check if the key exists.
    /// </summary>
    private TimeSpan GetAge(TKey key, DateTime now)
    {
        return now.Subtract(Cache[key].CachedOn);
    }

    /// <summary>
    /// Refreshes the value of the cache item with the indicated key by executing its RefreshCache function expression.
    /// </summary>
    private void Refresh(TKey key, DateTime now)
    {
        if (!Cache.ContainsKey(key))
        {
            throw new ArgumentOutOfRangeException("Cache doesn't contain supplied key.");
        }

        if (Cache[key].RefreshCache == null)
        {
            throw new NullReferenceException("RefreshCache function expression is null.");
        }

        var oldValue = Cache[key].Value;

        Cache[key].Value = Cache[key].RefreshCache();
        Cache[key].CachedOn = now;

        if (!oldValue.Equals(Cache[key].Value))
        {
            OnChanged(key);
        }
    }


    /// <summary>
    /// Sets the expiration of the cache item to the indicated expiration.
    /// </summary>
    public void SetExpiration(TKey key, TimeSpan expiration)
    {
        if (!Cache.ContainsKey(key))
        {
            return;
        }

        var oldExpiration = Cache[key].Expiration;
        Cache[key].Expiration = expiration;
        OnExpirationChanged(key, oldExpiration, expiration);
    }


    /// <summary>
    /// A key/value pair that also has a cached timestamp, expiration, and function for refreshing the value.
    /// </summary>
    private class CacheItem
    {
        public TKey Key { get; set; }
        public TValue Value { get; set; }
        public DateTime CachedOn { get; set; }
        public TimeSpan Expiration { get; set; }
        public Func<TValue> RefreshCache { get; set; }
    }
}

Things I'm looking for include,

• Improved naming, readability, and reuse.
• Improved performance but not at the cost of above (if possible).
• Any tips on events and delegates (I don't use these often).

Answer 1

CacheItem

You're not really using the Key property of the CacheItem class, you're always operating with the parameter, you can either remove the property as it's redundant or use that instead of the parameter.

Accessing Dictionary<TKey,TValue> properly

When you want to check if your dictionary contains a key and if it does do something with the value, the proper way to do it is to use TryGetValue(TKey key, out CacheItem value) to save the result in a variable instead of accessing the dictionary all the time.

This will be faster, cleaner and accesses the dictionary less times:

public TValue Get(TKey key, bool isForcingRefresh = false)
{
    CacheItem item;
    if (!Cache.TryGetValue(key, out item))
    {
        throw new ArgumentOutOfRangeException("Cache doesn't contain supplied key.");
    }

    var now = DateTime.Now;
    var expiration = item.Expiration;
    if (isForcingRefresh || GetAge(item.Key, now) >= expiration)
    {
        OnRefreshed(item.Key);
        Refresh(item.Key, now);
    }

    OnRetrieved(item.Key);
    return item.Value;
}

You have some more methods that can be changed in a similar way.

Superfluous Expression

Expression<Func<TValue>> refreshCache can be Func<TValue> refreshCache and just assign that directly to RefreshCache, that's instant where .Compile() takes some computation time.

Events and delegates

• You can shorten the invocation of events by using null propagation:

  if (ExpirationChanged != null)
  {
      ExpirationChanged(this, key, oldExpiration, newExpiration);
  }
  

  Can become:

  ExpirationChanged?.Invoke(this, key, oldExpiration, newExpiration);
  

  But you should still keep your protected methods. Tho they're usually virtual protected.

Handling invalid input

If your methods will be used by outside classes and potentially another user, you should make sure you don't allow invalid input and if there is invalid input, either throw an exception (preferably) or just return from the method.

Either way you should be consistent with what you pick, unless there is a clear indication that this method wont throw an exception in case of invalid input and will instead just exit.

For example SetExpiration() is somewhat 'exception' to the way your other methods function and deal with invalid arguments, they all just throw an exception but this one just exits the method.

Repetitive procedures

Your Refresh(TKey key...) method is being used just in 1 place namely in your Get(TKey key...) method.

Look at this line Refresh(item.Key, now);, after this your Refresh method proceeds to look for that exact same item that we just got the key from. What you can do is, create another overload that will accept CacheItem item instead or just edit the current one and get rid of the overload that accepts TKey as it wont be used anyway.

private void Refresh(CacheItem item, DateTime now)
{
    if (item.RefreshCache == null)
    {
        throw new NullReferenceException("RefreshCache function expression is null.");
    }

    var oldValue = item.Value;

    item.Value = item.RefreshCache();
    item.CachedOn = now;

    if (!oldValue.Equals(item.Value))
    {
        OnChanged?.Invoke(this, item.Key);
    }
}

Naming

Private variables should follow the camelCase typing convention:

Cache -> (cache or _cache);

_DefaultExpiration -> (_defaultExpiration or defaultExpiration)

In your Refresh method, I don't think DateTime now is a good name, that hints that you're talking about DateTime.Now but why bother passing it as parameter if it's constant?