patterncsharpMinor
A Simple Cache class
Viewed 0 times
cachesimpleclass
Problem
“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
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 _cache = new Dictionary();
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; }
}
}Solution
- 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.
- 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.
- 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.
- Make the magic number
30000at least a const member or maybe even a tunable property/field (in which case you should prefer making it aTimeSpanas it's easier to use than plain ticks)
- Your
Addmethod 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().
- Depending on your use case you might want to rename
CreatedintoLastAccessedand 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).
- Last but not least you might want to look into the
MemoryCacheclass if you are using .NET 4 or later.
Context
StackExchange Code Review Q#35864, answer score: 8
Revisions (0)
No revisions yet.