7
\$\begingroup\$

“There are only two hard things in Computer Science: cache invalidation and naming things.” -- Phil Karlton

That being said, I created this SimpleCache class which I intend to use to cache database queries and results over a relatively short period of time.

I'm just asking if there are any immediate faults or errors with this?

(The class is internal because I'm using it inline in another class)

internal sealed class SimpleCache
{
    private static Dictionary<string, CacheItem> _cache = new Dictionary<string, CacheItem>();

    public void Add(string key, object value)
    {
        _cache.Add(key, new CacheItem() { Value = value, Created = DateTime.Now.Ticks });
    }

    public object this[string key]
    {
        get
        {
            if(_cache.ContainsKey(key))
            {
                if ((_cache[key].Created + 30000) > DateTime.Now.Ticks) //30000 was chosen by magic
                {
                    return _cache[key].Value;
                }
                    _cache.Remove(key);
            }
            return null;
        }
    }

    internal sealed class CacheItem
    {
        public object Value { get; set; }
        public long Created { get; set; }
    }
}
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Use the built in MemoryCache class! \$\endgroup\$ – Peter Kiss Nov 21 '13 at 18:14
8
\$\begingroup\$
  1. You have a static dictionary as backing store which means that all cache instances share it. Not saying that this is wrong and it might be intended but you should be aware of it.
  2. Your cache apparently is not thread-safe. You need to protect access to the dictionary with locks or if you are using .NET 4 or later use ConcurrentDictionary. If you make the backing dictionary non-static then you might not need to make it thread-safe if you can ensure that you never access the same cache instance from multiple threads at the same time.
  3. Items only get evicted when they are being accessed again. This means if you only ever access items once they will stay in there forever.
  4. Make the magic number 30000 at least a const member or maybe even a tunable property/field (in which case you should prefer making it a TimeSpan as it's easier to use than plain ticks)
  5. Your Add method will throw an exception if the entry in the cache already exists. You either need to check if it exists and just update the entry (or do nothing, whatever makes sense) or replace the entry by using _cache[key] = .. rather than _cache.Add().
  6. Depending on your use case you might want to rename Created into LastAccessed and update it whenever someone accesses the entry (read/write) with the same key (which means items will be evicted a certain period after they have been last accessed rather than created).
  7. Last but not least you might want to look into the MemoryCache class if you are using .NET 4 or later.
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Not all classes have to be thread-safe. Though static things should. \$\endgroup\$ – svick Nov 21 '13 at 19:01
  • \$\begingroup\$ Good points all of them. I actually implemented bullet point #1 (changed it to non-static), #2, #4 (except I kept it as a long) and #6. Making it a ConcurrentDictionary also takes care of #5 as in .TryAdd() will return false if the key already exists. \$\endgroup\$ – Max Nov 22 '13 at 9:37

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.