Sort a list for a GridView I've written the following piece of code to sort my list, which is the data source to my GridView, depending on the column to be sorted on. It seems to me that there is quite a bit of redundancy, and the below code may be optimized. Also, I wanted to apply the strategy pattern (note that I'm just starting out with design patterns), but it seems to me like it would be overkill. Please suggest how the following may be better written.

User is my class with several properties, including but not limited to Role, Policy, and Name. The other properties may also be included in this method, so it might grow. SortingOrder is an enum with 2 values, Ascending and Descending. SortExpression contains the value based on which the data is to be sorted.

    private void SortUsers(ref List&lt;User&gt; userList)
    {
        if (SortOrder == SortingOrder.Ascending)
        {
            switch (SortExpression)
            {
                case "Role":
                    userList = userList.OrderBy(user =&gt; user.Role).ToList();
                    break;
                case "Policy":
                    userList = userList.OrderBy(user =&gt; user.Policy).ToList();
                    break;                   
                default:
                    userList = userList.OrderBy(user =&gt; user.Name).ToList();
                    break;
            }
        }
        else
        {
            switch (SortExpression)
            {
                case "Role":
                    userList = userList.OrderByDescending(user =&gt; user.Role).ToList();
                    break;
                case "Policy":
                    userList = userList.OrderByDescending(user =&gt; user.Policy).ToList();
                    break;                  
                default:
                    userList = userList.OrderByDescending(user =&gt; user.Name).ToList();
                    break;
            }
        }
    }
Repeated Switches

I'm not sure what you mean by Strategy Pattern (never seen the term before), but judging by Wikipedia, I imagine you are referring to the idea of replacing the pair of switch statements, which are liable to fall out of sync and create hard-to-find bugs, with a method to look up the key (e.g. a mapping from "Role" to user =&gt; user.Role and such) and then just switch on the SortingOrder, or a method to perform the sort itself (I might be inclined to do both). This is a great idea!

This can be as complex or simple as you like, because it is abstracted away. It should pay off to simply move the existing switch into a method:



delegate IComparable UserKey(User user);

private static UserKey TranslateSortExpression(string sortExpression)
{
    switch (sortExpression)
    {
        case "Role":
            return user =&gt; user.Role;
        case "Policy":
            return user =&gt; user.Policy;
        default:
            return user =&gt; user.Name;
    }
}

private void SortUsers(ref List&lt;User&gt; userList)
{
    UserKey sortingKey = TranslateSortExpression(SortExpression);

    if (SortOrder == SortingOrder.Ascending)
    {
        userList = userList.OrderBy(sortingKey).ToList();
    }
    else
    {
        userList = userList.OrderByDescending(sortingKey).ToList();
    }
}


A non-obvious benefit of pulling out the expression translator is that by having it return values, the compiler will ensure that it always returns a result. Currently, if you'd added an incomplete code-path by accident you may not notice, as your code would silently fail.

Anyhow, the code remains simple, has definitely become more maintainable, and now you have an obvious place to modify the code (e.g. to if you wanted to look up the Key delegate from a dictionary (which could allow you to have 'user keys' or all manner of extensibility)).

ref List&lt;User&gt;

It is clear why you have done this, but it is a tad scary. My first assumption was that this code was performing an in-place sort, and why it is ref isn't necessary clear to the caller (though thankfully C# makes you acknowledge this at the call site).

Unless you have good reason to duplicate the list, you should consider an inplace sort (e.g. using List.Sort(...), though this requires writing a comparer). If not, I would consider changing the method signature so that it takes in one list and returns another. As lovely as ref variables are, explicitly returning the new list will encourage cleaner calling code (i.e. not reusing a variable name for the unsorted and then sorted list) and make the method behaviour more obvious.

Either way, you should considering adding inline documentation to this method (\\\ &lt;summary&gt; etc.) to remind people of the chosen behaviour.

As before, returning also gives the compiler a chance to check you haven't omitted anything. Imagine the misery that could stem from no replacing the list, and finding that some backing list you are expecting to be cloned is being mysteriously modified by some code that is operating on the sorted list (I'm not saying I expect your code will do this, but it's the sort of misery inducing error that can take all day to debug because the code doesn't fail quickly (I'm a big proponent of fail-fast: the best time to fail is at compile time)).

Furthermore, if you return a different list, it can be of a different type. If your signature looked like

private void IList&lt;User&gt; SortUsers(IReadOnlyList&lt;User&gt; unsortedUserList);


then the behaviour becomes apparent (Is it an inplace sort? No, I pass it a readonly list! Where is the output? It's a return value!)

I'll throw in a comment here about using interfaces instead of concrete implementations (e.g. IList&lt;T&gt; instead of List&lt;T&gt;) where possible. It allows people to use their own implementations (which keeps code clean and potentially more efficient) and to exploit type-variant generics (interfaces can be variant, but types cannot (IList&lt;T&gt; is invariant, so it doesn't apply here (though IReadOnlyList&lt;T&gt; is covariant))

A word about enums

You should be made aware (if you are not already) that C# enums are not type safe (they are pretty dreadful). You might consider adding an explicit check for SortingOrder.Descending and throwing if any other value is passed in (e.g. (SortingOrder)12)

Instance method

Is there a good reason this is an instance method and not a static method? If you can't think of one, then it probably should be static, otherwise another programmer may start to wonder why. Static methods just 'look' honest, because they can't change the state of instances unexpectedly (atleast, not in sane systems).

I don't know enough about the layout of your code to say whether this should or should not be static, but it's worth pondering if you haven't though about it yet.

(I note that SortExpression isn't a parameter, so it is presumably an instance member, which may be a good reason to keep this virtual, but again, not enough info in the question for us really to comment on this)