HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Thread safe cache for write through and writeback

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
writebackwritecachethreadforsafethroughand

Problem

This cache is like a list where new elements are inserted in the middle, cache hits are put to head of the list and replaced elements are taken from the end. Would I just use a list the lookup would take O(n). With a dictionary it is O(log(n)) but I lose the order of elements, so I use both.

.Net but only has ConcurrentDictionary but not a ConcurrentList so I did not mix it with a List. Even read access would end in a write (cache hit) so I just use a lock in every method. How could I achieve something like multiple reader - one writer locking?

```
using System;
using System.Collections.Generic;
using System.Diagnostics;

namespace Bodhi
{
///
/// Simple threadsafe caching collection
///
/// Key of the cached item implementing IComparable
/// Cached item
public class BodhiCache
where TKey : IComparable
where TItem : class
{
///
/// Initializes a new instance of BodhiCache.
///
/// Capacity, default 1024
public BodhiCache(int capacity = 1024)
{
this.Capacity = capacity;

m_keyOrder = new List(this.Capacity);
m_cache = new Dictionary(this.Capacity);
}

public int Capacity { get; private set; }

///
/// Returns the current cache content including keys and items.
///
public Dictionary Content
{
get
{
lock (m_syncRoot)
{
return new Dictionary(m_cache);
}
}
}

///
/// Gets the current number of cached items
///
public int Count
{
get { return m_keyOrder.Count; }
}

///
/// Clears the cache. Get the content before hand to implement a cache flush.
///
public void Clear()
{
lock (m_syncRoot)
{
Debug.Assert(m_keyOrder.Count == m_c

Solution

Don't bury all of your private fields at the bottom of the file. I almost voted to close this question for being broken because I thought they weren't defined anywhere. Most developers will expect to see them defined at the top of the class. Also ditch the m_ prefix. It's unnecessary noise. Use an underscore if you like, but no more. Even using an underscore to denote private fields is a debatable topic.

You also missed filling in some documentation here.

/// 
    /// Invalidate a cache item by key
    /// 
    /// 


XML doc comments are great, but only if you fill them in completely.

I don't like your constructor either.

public BodhiCache(int capacity = 1024)
    {
        this.Capacity = capacity;

        m_keyOrder = new List(this.Capacity);
        m_cache = new Dictionary(this.Capacity);


I'm wary of any constructor that "news up" anything. It makes it hard to test and inject dependencies. I would also be wary of using optional arguments. Now, I understand you're just making sure the class has what it needs to function, but you should carefully consider if you should have a default constructor and an overloaded one that takes a list, dict, and capacity in as dependencies.

I would write your constructor more like this.

//default, paramaterless contructor
public BodhiCache()
    :this(1024)
{}

public BodhiCache(int capacity)
    :this(capacity, new List(capacity), new Dictionary(capacity))
{} 

public BodhiCache(int capacity, IList keyOrder, Dictionary)
{
    _this.Capacity = capacity;
    _keyOrder = keyOrder;
    _cache = cache;
}


Sorry I didn't address your actual concerns, but hopefully this was still helpful.

Code Snippets

/// <summary>
    /// Invalidate a cache item by key
    /// </summary>
    /// <param name="key"></param>
public BodhiCache(int capacity = 1024)
    {
        this.Capacity = capacity;

        m_keyOrder = new List<TKey>(this.Capacity);
        m_cache = new Dictionary<TKey, TItem>(this.Capacity);
//default, paramaterless contructor
public BodhiCache()
    :this(1024)
{}

public BodhiCache(int capacity)
    :this(capacity, new List<TKey>(capacity), new Dictionary<TKey, TItem>(capacity))
{} 

public BodhiCache(int capacity, IList<TKey> keyOrder, Dictionary<TKey, TItem>)
{
    _this.Capacity = capacity;
    _keyOrder = keyOrder;
    _cache = cache;
}

Context

StackExchange Code Review Q#83375, answer score: 3

Revisions (0)

No revisions yet.