Session state wrapper, extending an existing application

Question

I'm adding additional functionality to an existing MVC .net application, and to help prevent or at least reduce repeated reads to the dB I'm dumping a few custom entities in session. I'm limiting what can be stored in session to say a max of 5 objects for now.

Given below is the code and it seems generic enough and works fine locally on my box; any suggestions to improve this and session is In-Proc but if it does get stored in a persistent medium say SQL server then I would need to customize this further - decorate custom object properties w/XML element attributes

public class QuestionModelSessionStateProvider : IQuestionModelSessionStateProvider
{
    const int MaxSize = 5;

    public T GetValue<T>(string key)
    {
        HttpSessionState session = GetSessionState();

        T returnValue = default(T);

        if (session == null) return returnValue;

        if (session["QuestionModelStore"] != null)
        {
            var questionModelStore = (List<KeyValuePair<string, T>>)session["QuestionModelStore"];
            foreach (var t in questionModelStore.Where(t => t.Key.Equals(key)))
            {
                returnValue = t.Value;
            }
        }

        return returnValue;
    }


    public void SetValue<T>(string key, T value)
    {
        HttpSessionState session = GetSessionState();

        if (session == null) return;

        List<KeyValuePair<string, T>> questionModelStore;

        if (session["QuestionModelStore"] == null)
        {
            questionModelStore = new List<KeyValuePair<string, T>> { new KeyValuePair<string, T>(key, value) };
            session["QuestionModelStore"] = questionModelStore;
        }
        else
        {
            questionModelStore = (List<KeyValuePair<string, T>>)session["QuestionModelStore"];
            questionModelStore.Add(new KeyValuePair<string, T>(key, value));
        }

        if (questionModelStore.Count > MaxSize)
        {
            questionModelStore.RemoveAt(0);
        }
    }


    public void RemoveValue<T>(string key)
    {
        HttpSessionState session = GetSessionState();
        int indexToRemoveAt = 0;
        var found = false;
        if (session != null && session["QuestionModelStore"] != null)
        {
            var questionModelStore = (List<KeyValuePair<string, T>>) session["QuestionModelStore"];
            for (var i = 0; i < questionModelStore.Count; i++)
            {
                if (questionModelStore[i].Key.Equals(key))
                {
                    indexToRemoveAt = i;
                    found = true;
                    break;
                }
            }

            if (found)
                questionModelStore.RemoveAt(indexToRemoveAt);
        }
    }


    public void Clear()
    {
        HttpSessionState session = GetSessionState();
        if (session != null)
        {
            session["QuestionModelStore"] = null;
        }
    }


    public int Count<T>()
    {

        HttpSessionState session = GetSessionState();
        if (session != null)
        {
            //return session.Count;
            if (session["QuestionModelStore"] != null)
            {
                var questionModelStore = (List<KeyValuePair<string, T>>)session["QuestionModelStore"];
                return questionModelStore.Count;
            }

        }

        return 0;

    }



    private static HttpSessionState GetSessionState()
    {
        if (HttpContext.Current != null)
            return HttpContext.Current.Session;
        return null;
    }
}

Answer 1

There are 4 places where you have code like,

if (session != null)
{
    List<KeyValuePair<string, T>> questionModelStore;

    if (session["QuestionModelStore"] == null)
    {
        questionModelStore = (List<KeyValuePair<string, T>>)session["QuestionModelStore"];

It would be neater if these were a common subroutine:

List<KeyValuePair<string, T>> GetQuestionModelStore<T>()
{
    var session = HttpContext.Current.Session;
    if (!session)
        return null;
    return (List<KeyValuePair<string, T>>)session["QuestionModelStore"];
}

That would simplify code such as:

public T GetValue<T>(string key)
{
    var cached = GetQuestionModelStore();
    if (cached == null)
        return default(T);
    return cached.FirstOrDefault(t => t.Key.Equals(key));
}

It might be slightly faster to use a Queue than a List, because a Queue is designed to let you enqueue at one end and dequeue at the other.

You're not quite implementing a LRU algorithm: if you want to do that, when you find an item in the cache you should move the item to the most-recently-used end of the list.

There's one bug I see, which is that your methods are generic: the first time you call SetValue it creates a List<KeyValuePair<string, T>>; unless all your SetValue and GetValue methods use the same type of T this will cause a run-time error (casting between different types of T). If all your SetValue and GetValue methods do use the same type of T then you might as well specify what T is instead of making it generic.