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

Imposing a sort order on a query

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

Problem

I've always been told "never use goto, there's always a better way" and for the longest time I just accepted it. Lately though, I've been running into such scenarios in which I have to repeat this bit of code every time I return early.

I personally hate repeating myself, even the littlest bit of code. So I ask, Is this use of goto really that bad? It prevents me from having to write that bit of code 3 times and sure, theoretically I could write some guard clauses (that could be combined into a single guard clause) that prevents the need for the try/catch and therefore having a single place for that bit of code, but would that actually be more readable?

public static IQueryable Sort(this IQueryable query, String field, String direction, Expression> defaultSort = null)
{
    if (String.IsNullOrWhiteSpace(field)) { goto FAILED; }

    try
    {
       // Dynamic LINQ - Field: "SomeProperty.OptionalProperty", Direction: "Desc" or "Asc"
        query = query.OrderBy(String.Format("{0} {1}", field, direction));
    }
    // NOT FOUND: Field wasn't found just return the original query.
    catch (NullReferenceException)
    {
        goto FAILED;
    }
    // NOT FOUND: Field wasn't found just return the original query.
    catch (ParseException)
    {
        goto FAILED;
    }

    return query;

    // SOMETHING FAILED: Return the original query with the optionally supplied default sort.
    FAILED:
        if (defaultSort != null)
        {
            query = query.OrderBy(defaultSort);
        }
        return query;
}

Solution

I'm not a member of the “goto is evil and must not be used under any circumstances” camp. Especially in performance-critical low-level routines, it can be useful from time to time. However, most of the time, the temptation to use goto actually stems from missing a better opportunity to structure the code. In your example, I'd simply re-structure it like this, which I find is not only more readable but also much shorter.

public static IQueryable
Sort(this IQueryable query,
        String field,
        String direction,
        Expression> defaultSort = null)
{
    if (!String.IsNullOrWhiteSpace(field)) {
        try {
            return query.OrderBy(String.Format("{0} {1}", field, direction));
        } catch (NullReferenceException) {
            // NOT FOUND: Field wasn't found just return the original query.
        } catch (ParseException) {
            // NOT FOUND: Field wasn't found just return the original query.
        }
    }
    if (defaultSort != null) {
        query = query.OrderBy(defaultSort);
    }
    return query;
}

Code Snippets

public static IQueryable<T>
Sort<T>(this IQueryable<T> query,
        String field,
        String direction,
        Expression<Func<T, Int32?>> defaultSort = null)
{
    if (!String.IsNullOrWhiteSpace(field)) {
        try {
            return query.OrderBy(String.Format("{0} {1}", field, direction));
        } catch (NullReferenceException) {
            // NOT FOUND: Field wasn't found just return the original query.
        } catch (ParseException) {
            // NOT FOUND: Field wasn't found just return the original query.
        }
    }
    if (defaultSort != null) {
        query = query.OrderBy(defaultSort);
    }
    return query;
}

Context

StackExchange Code Review Q#95265, answer score: 19

Revisions (0)

No revisions yet.