2
\$\begingroup\$

I sometimes need an object that caches some data but only for the specified amount of time. So I created a class that should handle this. The main goal was to make it generic unlike the MemoryCache and be a single object cache and not a general all-pupose cache.

ObjectCache

public class ObjectCache<T>
{
    private readonly object _valueExpiredMutex = new object();

    private T _value;

    public ObjectCache(TimeSpan expiration)
    {
        Expiration = expiration;
    }

    public EventHandler<ObjectCacheEventArgs<T>> ValueExpired { get; set; }

    public TimeSpan Expiration { get; set; }

    public DateTime? LastUpdate { get; internal set; }

    public bool IsExpired
    {
        get { return !LastUpdate.HasValue || DateTime.Now - LastUpdate.Value > Expiration; }
    }

    public T GetValue()
    {
        if (!IsExpired)
        {
            return _value;
        }

        lock (_valueExpiredMutex)
        {
            _value = OnValueExpired();
            LastUpdate = DateTime.Now;
        }

        return _value;
    }

    private T OnValueExpired()
    {
        if (ValueExpired == null)
        {
            throw new InvalidOperationException("ValueExpired event handler not set.");
        }

        var e = new ObjectCacheEventArgs<T>();
        ValueExpired(this, e);
        return e.Value;
    }
}
  • LastUpdate has an internal setter so that I can set it in tests
  • I tried to implement it to be thread-safe but I'm not sure if I did it correctly with only one mutex for getting a new value.
  • Any kind of feedback is welcome ;-)

EventArgs

public class ObjectCacheEventArgs<T> : EventArgs
{
    public T Value { get; set; }
}

Test

[TestMethod]
public void TestObjectCache()
{
    var number = 1;

    var objectCache = new ObjectCache<int>(
        new TimeSpan(hours: 0, minutes: 21, seconds: 0));
    objectCache.ValueExpired += (sender, e) =>
    {
        e.Value = number++;
    };
            
    Assert.IsTrue(objectCache.IsExpired);
    Assert.AreEqual(1, objectCache.GetValue());
    Assert.IsFalse(objectCache.IsExpired);

    objectCache.LastUpdate = DateTime.Now.AddMinutes(-30);
    Assert.IsTrue(objectCache.IsExpired);
    Assert.AreEqual(2, objectCache.GetValue());
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You probably saw my comment to Heslacher but you really should replace internal timings using DateTime.Now with DateTime.UtcNow, which is not only faster but also not prone to unexpected behavior during time zone transitions. \$\endgroup\$
    – Rick Davin
    Oct 27, 2015 at 14:13
  • \$\begingroup\$ @RickDavin oops, right, yes, I'll implement this change too ;-) \$\endgroup\$
    – t3chb0t
    Oct 27, 2015 at 14:14

1 Answer 1

2
\$\begingroup\$

IMO this a strange aproach. One needs to first call GetValue() to be able to set a value by using an eventhandler. I would expect from such an ObjectCache to provide a constructor which takes a value as parameter too.

In addition I would add a property to ObjectCacheEventArgs<T> signaling a state, for instance an enum having the members ChangeValue, KeepAlive and SkipValue to be set by the handler of the event.
If this value is e.g set to ChangeValue then the _value should be changed. If it is set to KeepAlive the LastUpdate should be set to DateTime.Now. And for SkipValue you would need to add another var indicating that the value is skipped and set _value to default(T).

It isn't obvious from a caller side of view that you have to add an eventhandler for that event. IMO this is a bad sign which one would maybe notice first at running the code. Such behaviour shouldn't be wanted and isn't expected. If one does not add an eventhandler he expects that he just don't get notified if this event happens.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ I'd suggest that LastUpdate be set to DateTime.UtcNow instead of DateTime.Now. All internal time keeping should be Utc. \$\endgroup\$
    – Rick Davin
    Oct 27, 2015 at 13:01
  • \$\begingroup\$ The idea is to let the event handler retrieve the value (usualy from an expensive database call) and use it for some time. If if force the user to set an initial value via the constructor he would need to retrieve the value twice, once for the constructor and the second time in the event handler. But I think I will redesign it so the constructor at least requires a Fun<state, T> method instead of an event. Then it shouldn't be easier to use it. Thank for your thougts ;-) \$\endgroup\$
    – t3chb0t
    Oct 27, 2015 at 13:34
  • \$\begingroup\$ What would you say if I changed the constructor to take this delegate as a second parameter? public delegate bool UpdateObjectCacheValue<T>(out T newValue); ;-) \$\endgroup\$
    – t3chb0t
    Oct 27, 2015 at 14:23
  • \$\begingroup\$ Looks fine to me. You could also just add it as an overloaded constructor so you won't break existing usage and refactor these later. \$\endgroup\$
    – Heslacher
    Oct 27, 2015 at 14:35
  • \$\begingroup\$ @Heslacher fortunately it's brand new and only one test application uses it so far so I'll get rid of the old construtor right away. \$\endgroup\$
    – t3chb0t
    Oct 27, 2015 at 20:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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