HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Ordering filtered results

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
resultsorderingfiltered

Problem

I have the following method which under code review gives me a CA1502 avoid excessive complexity error (although if I suppress the message it will build fine):

private void OrderResults()
{
    switch (_orderColumn)
    {
        case 0:
        default:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;

        case 1:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;

        case 2:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.City) : _fitleredResults.OrderBy(x => x.City);
            break;

        case 3:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Country) : _fitleredResults.OrderBy(x => x.Country);
            break;

        case 4:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.RecID) : _fitleredResults.OrderBy(x => x.RecID);
            break;
    }
}


Explanation of variables:

  • _filteredResults is an IEnumerable;



  • _orderColumn is an int passed in by jQuery DataTables and doesn't match the property indexes (which is why I was mapping it using a switch)



Address object class:

public class AddressObject
{
    public string City { get; set; }
    public string Country { get; set; }
    public string CountryRegionID { get; set; }
    public string Postcode { get; set; }
    public long RecID { get; set; }
    public string Street { get; set; }
}


Is there a way to make this code less

Solution

Firstly, your case 1 case is identical to your case 0,default case (order by street). Is that intentional? It looks like a mistake to me because you've cascaded 0 and default but not 1. If it isn't a mistake, and they do all have the same behaviour you should cascade them all or let them all be picked up by the default case.

I normally see default as the last option in a switch statement, I feel like that might be a convention but don't have anything to back it up.

You're repeating this code a lot: _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) put it in a variable (which will make the code more readable.

So far:

private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    switch (_orderColumn)
    {    
        case 2:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.City) : _fitleredResults.OrderBy(x => x.City);
            break;
        case 3:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Country) : _fitleredResults.OrderBy(x => x.Country);
            break;
        case 4:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.RecID) : _fitleredResults.OrderBy(x => x.RecID);
            break;
        default:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;
    }
}


As Heslsacher says, you could further improve by using named constants for the indexes.

I did write some additional stuff about using a method to get a Func but then I realised that RectId is a long so it all went out the window.

Edit

For posterity, I'll add in what I had written:

private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    var sortProperty = GetSortProperty();
    if (isDescending)
    {
        _filteredResults = _filteredResults.OrderByDescending(sortProperty);
    }
    else
    {
        _filteredResults = _filteredResults.OrderBy(sortProperty);
    }
}

private Func GetSortProperty()
{
    switch(_orderColumn)
    {
        case 2:
            return x => x.City;
        case 3:
            return x => x.Country;
        case 4:
            return x => x.RectId;
        default: 
            return x => x.Street;
    }
}

Code Snippets

private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    switch (_orderColumn)
    {    
        case 2:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.City) : _fitleredResults.OrderBy(x => x.City);
            break;
        case 3:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Country) : _fitleredResults.OrderBy(x => x.Country);
            break;
        case 4:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.RecID) : _fitleredResults.OrderBy(x => x.RecID);
            break;
        default:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;
    }
}
private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    var sortProperty = GetSortProperty();
    if (isDescending)
    {
        _filteredResults = _filteredResults.OrderByDescending(sortProperty);
    }
    else
    {
        _filteredResults = _filteredResults.OrderBy(sortProperty);
    }
}

private Func<AddressObject, object> GetSortProperty()
{
    switch(_orderColumn)
    {
        case 2:
            return x => x.City;
        case 3:
            return x => x.Country;
        case 4:
            return x => x.RectId;
        default: 
            return x => x.Street;
    }
}

Context

StackExchange Code Review Q#93746, answer score: 5

Revisions (0)

No revisions yet.