Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

After reading a bit about the System.Runtime.Caching in the .net 4 framework I've added a simple Cache class to my application.

Can I improve it or add additional useful functionality? Typing it out here I see that the cache checking is somewhat duplicated; could this be refactored?

Note: the two properties SuperUsers and TemplateLocation are just for an example of how the class would be used. Most properties would be a string or list.

/// <summary>
/// Cache class as singleton to make those often used 
/// values that may change just that bit more speedy.
/// </summary>
/// <remarks>
/// Jon Skeets' singleton http://csharpindepth.com/Articles/General/Singleton.aspx
/// </remarks>
public sealed class Cacher
{
    private readonly ObjectCache _cache = MemoryCache.Default;
    public CacheItemPolicy Policy { get; set; }

    private static readonly Cacher instance = new Cacher();

    static Cacher()
    { }

    private Cacher()
    {
        // expiration policy default
        Policy = new CacheItemPolicy 
        {
            AbsoluteExpiration = DateTimeOffset.Now.AddMinutes(10.0)
        };
    }

    public static Cacher Instance
    {
        get { return instance; }
    }

    public string TemplateLocation
    {
        get 
        { 
            var loc = _cache["templatelocation"] as string;
            if (string.IsNullOrEmpty(loc))
            {
                // hardcoded for test purposes
                _cache.Set("templatelocation", getTemplateLocation(), Policy);
                loc = _cache["templatelocation"] as string;
            }
            return loc;
        }
    }

    public List<string> SuperUsers
    {
        get
        {
            var superUsers = _cache["superusers"] as List<string>;
            if (superUsers == null)
            {
                // empty, so stick values in the cache
                _cache.Set("superusers", GetSuperUsers(), Policy);
                superUsers = _cache["superusers"] as List<string>;
            }
            return superUsers;
        }
    }

    private string getTemplateLocation()
    {
        // hardcoded for test purposes
        return @"C:\";
    }

    private List<string> GetSuperUsers()
    {
        // hardcoded for test purposes
        return new List<string> {"John", "Anne", "Mark"};
    }

    public void Clear()
    {
        foreach (var cacheitem in _cache)
            _cache.Remove(cacheitem.Key);
    }
}

and using the class:

string location = Cacher.Instance.TemplateLocation;
List<string> sUsers = Cacher.Instance.SuperUsers;
share|improve this question
    
Is this supposed to be a caching class for just those two objects superusers and template location? – Randy Mar 30 '12 at 12:40
    
no, i just used those as an example. I shall edit the code. – Rob Mar 30 '12 at 13:13
    
Some of your get properties do too much work for a property. They also have side-effects (caching) I would replace them with GetSomething(...) methods, or methods with even better names that do hint at side effects. – Leonid Apr 1 '12 at 3:25
up vote 2 down vote accepted

Something you might consider is switching it to be a static class instead of a singleton. The caching system itself is already performing the singleton process.

To add functionality, if you change it to a static class you can add helpers to extend caching directly onto the object(s) itself like myDictionary.Cache() and myDictionary.LoadFromCache().

Reflection frequently gets used where it shouldn't. However, you could decorate the functionality to auto-infer the "namespace.class.variablename" which removes magic stings entirely and makes the extended functionality more fluent. I hope this helps.

share|improve this answer
    
Thanks for the great suggestions, could you expand a little on the second point - about extending onto objects? – Rob Apr 3 '12 at 15:16
    
Sure. If on the static class you add a method like "public static void Cache(this Dictionary item, string saveAs)" then you can just call myDictionary.Cache("userlist"). (see msdn.microsoft.com/en-us/library/bb383977.aspx ) – Randy Apr 3 '12 at 19:22

What do you need the singleton for? You don't need it at all. Just use static members if there is always one instance of this class.

I think the responsibilities of storing keys and values, and of returning and creating cache values should be separated.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.