Looking for a better way to sort my List<T>

Question

I'm reviewing a piece of code I wrote not too long ago, and I just hate the way I handled the sorting - I'm wondering if anyone might be able to show me a better way.

I have a class, Holding, which contains some information. I have another class, HoldingsList, which contains a List<Holding> member. I also have an enum, PortfolioSheetMapping, which has ~40 or so elements.

It sort of looks like this:

public class Holding
{
    public ProductInfo Product {get;set;} 
    // ... various properties & methods ...
}

public class ProductInfo
{
    // .. various properties, methods... 
}

public class HoldingsList
{
    public List<Holding> Holdings {get;set;}
    // ... more code ...
}

public enum PortfolioSheetMapping
{
    Unmapped = 0,
    Symbol,
    Quantitiy,
    Price,
    // ... more elements ...
}

I have a method which can invoke the List to be sorted depending on which enumeration the user selects. The method uses a mondo switch statement that has over 40 cases (ugh!).

A short snippet below illustrates the code:

if (frm.SelectedSortColumn.IsBaseColumn)
{
    switch (frm.SelectedSortColumn.BaseColumn)
    {
        case PortfolioSheetMapping.IssueId:
            if (frm.SortAscending)
            {
                // here I'm sorting the Holding instance's
                // Product.IssueId property values...
                // this is the pattern I'm using in the switch...
                pf.Holdings = pf.Holdings.OrderBy
                  (c => c.Product.IssueId).ToList();
            }
            else
            {
                pf.Holdings = pf.Holdings.OrderByDescending
                  (c => c.Product.IssueId).ToList();
            }
            break;
        case PortfolioSheetMapping.MarketId:
            if (frm.SortAscending)
            {
                pf.Holdings = pf.Holdings.OrderBy
                  (c => c.Product.MarketId).ToList();
            }
            else
            {
                pf.Holdings = pf.Holdings.OrderByDescending
                  (c => c.Product.MarketId).ToList();
            }
            break;
        case PortfolioSheetMapping.Symbol:
            if (frm.SortAscending)
            {
                pf.Holdings = pf.Holdings.OrderBy
                  (c => c.Symbol).ToList();
            }
            else
            {
                pf.Holdings = pf.Holdings.OrderByDescending
                  (c => c.Symbol).ToList();
            }
            break;
        // ... more code ....

My problem is with the switch statement. The switch is tightly bound to the PortfolioSheetMapping enum, which can change tomorrow or the next day. Each time it changes, I'm going to have to revisit this switch statement, and add yet another case block to it. I'm just afraid that eventually this switch statement will grow so big that it is utterly unmanageable.

Can someone tell me if there's a better way to sort my list?

Answer 1

You're re-assigning the sorted data straight back to your pf.Holdings property, so why not bypass the overhead of OrderBy and ToList and just use the list's Sort method directly instead?

You could use a map to hold Comparison<T> delegates for all the supported sortings and then call Sort(Comparison<T>) with the appropriate delegate:

if (frm.SelectedSortColumn.IsBaseColumn)
{
    Comparison<Holding> comparison;
    if (!_map.TryGetValue(frm.SelectedSortColumn.BaseColumn, out comparison))
        throw new InvalidOperationException("Can't sort on BaseColumn");

    if (frm.SortAscending)
        pf.Holdings.Sort(comparison);
    else
        pf.Holdings.Sort((x, y) => comparison(y, x));
}

// ...

private static readonly Dictionary<PortfolioSheetMapping, Comparison<Holding>>
    _map = new Dictionary<PortfolioSheetMapping, Comparison<Holding>>
    {
        { PortfolioSheetMapping.IssueId,  GetComp(x => x.Product.IssueId) },
        { PortfolioSheetMapping.MarketId, GetComp(x => x.Product.MarketId) },
        { PortfolioSheetMapping.Symbol,   GetComp(x => x.Symbol) },
        // ...
    };

private static Comparison<Holding> GetComp<T>(Func<Holding, T> selector)
{
    return (x, y) => Comparer<T>.Default.Compare(selector(x), selector(y));
}

Answer 2

You could try reducing the switch to something like this:

    private static readonly Dictionary<PortfolioSheetMapping, Func<Holding, object>> sortingOperations = new Dictionary<PortfolioSheetMapping, Func<Holding, object>>
    {
        {PortfolioSheetMapping.Symbol, h => h.Symbol},
        {PortfolioSheetMapping.Quantitiy, h => h.Quantitiy},
        // more....
    };

    public static List<Holding> SortHoldings(this List<Holding> holdings, SortOrder sortOrder, PortfolioSheetMapping sortField)
    {
        if (sortOrder == SortOrder.Decreasing)
        {
            return holdings.OrderByDescending(sortingOperations[sortField]).ToList();
        }
        else
        {
            return holdings.OrderBy(sortingOperations[sortField]).ToList();                
        }
    }

You could populate sortingOperations with reflection, or maintain it by hand. You could also make SortHoldings accept and return an IEnumerable and remove the ToList calls if you don't mind calling ToList in the caller later. I'm not 100% sure that OrderBy is happy receiving an object, but worth a shot.

Edit: See LukeH's solution to keep things strongly typed.

Answer 3

Have you looked into Dynamic LINQ

Specifically, you could simply do something like:

var column = PortFolioSheetMapping.MarketId.ToString();
if (frm.SelectedSortColumn.IsBaseColumn)
{
    if (frm.SortAscending)
         pf.Holdings = pf.Holdings.OrderBy(column).ToList();
    else
         pf.Holdings = pf.Holdings.OrderByDescending(column).ToList();
}

Note: This does have the constraint that your enum match your column names, if that suits you.

EDIT

Missed the Product property the first time. In these cases, DynamicLINQ is going to need to see, for example, "Product.ProductId". You could reflect the property name or simply use a 'well-known' value and concat with the enum .ToString(). At this point, I'm just really forcing my answer to your question so that it at least is a working solution.

Answer 4

how about:

Func<Holding, object> sortBy;

switch (frm.SelectedSortColumn.BaseColumn)
{
    case PortfolioSheetMapping.IssueId:
        sortBy = c => c.Product.IssueId;
        break;
    case PortfolioSheetMapping.MarketId:
        sortBy = c => c.Product.MarketId;
        break;
    /// etc.
}

/// EDIT: can't use var here or it'll try to use IQueryable<> which doesn't Reverse() properly
IEnumerable<Holding> sorted = pf.Holdings.OrderBy(sortBy);
if (!frm.SortAscending)
{
    sorted = sorted.Reverse();
}

?

Not exactly the fastest solution, but it's reasonably elegant, which is what you were asking for!

EDIT: Oh, and with the case statement, it probably needs refactoring to a seperate function that returns a Func, not really a nice way to get rid of it entirely, but you can at least hide it away from the middle of your procedure !