How can I get rid of this smell? (refactoring switch statement)

Question

Yesterday i was playing with jQGrid plugin and ASP.NET. Everything turned out well, my grid is working now, but i have two methods, that make my code smell.

smelly methods:

private IOrderedEnumerable<Employee> GetOrderedEmployees(Column sortColumn, bool ascending)
    {
        switch (sortColumn)
        {
            case Column.Name:
                {
                    return GetOrderedEmployees(e => e.Name, ascending);
                }
            case Column.Salary:
                {
                    return GetOrderedEmployees(e => e.Salary, ascending);
                }
            default:
                {
                    return GetOrderedEmployees(e => e.ID, ascending);
                }
        }
    }

    private IOrderedEnumerable<Employee> GetOrderedEmployees<TSortKey>(Func<Employee, TSortKey> func, bool ascending)
    {
        return ascending ? Context.Employees.OrderBy(func) : Context.Employees.OrderByDescending(func);
    }

I can't find out, how to refactor them properly. It's seems the best solution is to return only lambdas (f.e return e=>e.Name) in the switch statement, but how can it be done?

Edit - in the switch statement ascending argument is passed 3 times. Isn't it a duplication?

Answer 1

I think that you may be going over the top here. Returning lambdas (IMO) is far more confusing than a simple switch statement or if;else if;else block. There may be a better way, but sometimes you do need to check a condition, especially with columns (I HATE working with ListViews, but they are necessary and often require this kind of code.)

I don't think that you are at the point in which you need to refactor anything. If that switch statement becomes a maintenance headache, than go right ahead, but not all switch statements constitute "code smell".

To address your code duplication concern, you can use the switch to get the e.?? value first, save that off and then call the function once at the end of the method.

Answer 2

There are a few patterns that might work here, but how about a Provider? You could create a Provider that provides a GetOrderEmployees, then pass in the Provider you want used instead of the sort column.

Edit - But I also thing the first poster's comment has a lot of merit - don't make a complex solution for something that's small potatoes anyway.

Answer 3

If you're just trying to reduce the duplication in your code:

private IOrderedEnumerable<Employee> GetOrderedEmployees(Column sortColumn, bool ascending)
{
    Func<Employee, TSortKey> func = e => e.ID;

    switch (sortColumn)
    {
        case Column.Name:
            {
                func = e => e.Name;
            }
        case Column.Salary:
            {
                func = e => e.Salary;
            }
    }

    return GetOrderedEmployees(func, ascending);
}

private IOrderedEnumerable<Employee> GetOrderedEmployees<TSortKey>(Func<Employee, TSortKey> func, bool ascending)
{
    return ascending ? Context.Employees.OrderBy(func) : Context.Employees.OrderByDescending(func);
}

(I'm not fluent in .NET; please forgive any syntax errors.)