2
\$\begingroup\$

After much prompting from this post I wanted to build a simple, in-memory, thread-safe cache.

The only caveat (as far as I was originally concerned) was the need for two different absolute expiration times for cached objects - those being based on a property of the item being cached (IsFailureItem). This is for a .NET Framework 4.6.1 solution.

I am primarily interested in anything which makes this either thread-unsafe, leak memory or simply bad practice. I know the class itself could be generically typed, but that is a decision for the future.

public class CacheItem
{
    public IEnumerable<DataItem> Response { get; set; }
    public bool IsFailureItem { get; set; }
}

public class CacheHelper
{
    public static CacheHelper Cache { get; set; }
    private static IMemoryCache InMemoryCache { get; set; }

    static CacheHelper()
    {
        Cache = new CacheHelper();
        InMemoryCache = new MemoryCache(new MemoryCacheOptions { });
    }

    private CacheHelper() { }

    public CacheItem this[string key]
    {
        get => InMemoryCache.Get<CacheItem>(key);
        set => InMemoryCache.Set<CacheItem>(key, value, value.IsFailureItem ? FailureCacheEntryOption : SuccessCacheEntryOption );
    }

    private static MemoryCacheEntryOptions FailureCacheEntryOption => new MemoryCacheEntryOptions()
    { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheFailureSecondsToLive) };

    private static MemoryCacheEntryOptions SuccessCacheEntryOption => new MemoryCacheEntryOptions()
    { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheSuccessSecondsToLive) };
}
\$\endgroup\$
4
  • \$\begingroup\$ Looks almost fine. InMemoryCache can be readonly field not property. Also you can remove setters from all other properties then introduce a constructor for CacheItem. \$\endgroup\$
    – aepot
    Jul 22 at 5:43
  • \$\begingroup\$ @aepot Thanks, changes added to post. \$\endgroup\$
    – Matt W
    Jul 22 at 7:34
  • 2
    \$\begingroup\$ Don't edit the code in the post. That's not allowed here because it's making reviews not suitable to the post. Please rollback the edits through the edit history. If you want to share the final solution, you may post it in a separate answer. Btw, you may accept the review below in case it was helpful. \$\endgroup\$
    – aepot
    Jul 22 at 7:47
  • 1
    \$\begingroup\$ Thank you @aepot - very wise. Reverted. \$\endgroup\$
    – Matt W
    Jul 22 at 7:52
3
\$\begingroup\$

No reason to have a public set on public static CacheHelper Cache { get; set; } not anyway to set it so why expose it?

I think CacheItem should be immutable. If calling Set with a CacheItem that IsFailureItem is true but later can be set to false, wouldn't effect time. Same with Response. Is it to be expected to be able to retrieve the CacheItem and update/replace the Response property?

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, changes applied in posted code. You're right about the immutability. \$\endgroup\$
    – Matt W
    Jul 22 at 7:35
1
\$\begingroup\$

Here is the code, after applying changes as advised by the wise commenters here:

public class CacheItem
{
    public IEnumerable<DataItem> Response { get; }
    public bool IsFailureItem { get; }

    public CacheItem(bool isFailureItem, IEnumerable<DataItem> response = null)
    {
        IsFailureItem = isFailureItem;
        Response = response;
    }
}

public class CacheHelper
{
    public static CacheHelper Cache { get; }
    private static readonly IMemoryCache InMemoryCache;

    static CacheHelper()
    {
        Cache = new CacheHelper();
        InMemoryCache = new MemoryCache(new MemoryCacheOptions { });
    }

    private CacheHelper() { }

    public CacheItem this[string key]
    {
        get => InMemoryCache.Get<CacheItem>(key);
        set => InMemoryCache.Set<CacheItem>(key, value, value.IsFailureItem ? FailureCacheEntryOption : SuccessCacheEntryOption );
    }

    private static MemoryCacheEntryOptions FailureCacheEntryOption => new MemoryCacheEntryOptions()
    { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheFailureSecondsToLive) };

    private static MemoryCacheEntryOptions SuccessCacheEntryOption => new MemoryCacheEntryOptions()
    { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheSuccessSecondsToLive) };
}
\$\endgroup\$
3
  • \$\begingroup\$ I would recommend in the constructor of CacheItem when setting Response to be 'Response = response ?? Enumerable.Empty<DataItem>()' I personally don't like null IEnumerables as linq will throw but on flip side setting value if someone passed in null would also be strange. Its a design choice you will need to make. To me I always lean to IEnumerables shouldn't be null \$\endgroup\$ Jul 22 at 14:05
  • \$\begingroup\$ Actually, it would be great to be able to force that parameter/property to be non-null, but that's not possible at present. I guess I will actually throw an InvalidArgumentException. What do you think? \$\endgroup\$
    – Matt W
    Jul 22 at 14:45
  • \$\begingroup\$ Actually, in retrospect, the IEnumerable<DataItem> response = null is there because it should only be null if bool isFailureItem is true. This is an application specific decision. \$\endgroup\$
    – Matt W
    Jul 22 at 14:53

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.