Imposing a sort order on a query

Question

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<T> Sort<T>(this IQueryable<T> query, String field, String direction, Expression<Func<T, Int32?>> 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;
}

Answer 1

You could use goto, but you can also just define a function, as the label returns a value anyways. For example, your label FAILED:, would become the following function.

public IQueryable<T> Failed()
{
    if (defaultSort != null)
    {
        query = query.OrderBy(defaultSort);
    }
    return query;
}

You shouldn't ever use goto for flow control, or anything in general, really.

Answer 2

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<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;
}

Answer 3

Goto's are a poor-man's function without the return semantics. Take your code, add a function, and bingo!

private static IQueryable<T> OrderedDefault(this IQueryable<T> query, Expression<Func<T, Int32?>> defaultSort = null)
{
        if (defaultSort != null)
        {
            return query.OrderBy(defaultSort);
        }
        return query;
}


public static IQueryable<T> Sort<T>(this IQueryable<T> query, String field, String direction, Expression<Func<T, Int32?>> defaultSort = null)
{
    if (String.IsNullOrWhiteSpace(field)) {
        return OrderedDefault(query, defaultSort);
    }

    try
    {
        query = query.OrderBy(String.Format("{0} {1}", field, direction));
    }
    // NOT FOUND: Field wasn't found just return the original query.
    catch (NullReferenceException)
    {
        return OrderedDefault(query, defaultSort);
    }
    // NOT FOUND: Field wasn't found just return the original query.
    catch (ParseException)
    {
        return OrderedDefault(query, defaultSort);
    }

    return query;
}

Answer 4

Despite Dijkstra's famous essay, goto does have its uses. However, in C#, it is very unlikely that you would encounter a situation where goto is truly the best option.

C# explicitly provides a set of specialized keywords like continue and break that are meant to be limited gotos designed for use in those specific cases that call for goto, and only them. Since these are much more limited than goto, they do not harm readability because they are much more predictable. With a goto end, who knows where the end is. But with continue, there's only one place it could ever possibly go.

The way you use goto, you're basically making a crummy subroutine. In your specific example, the only way the program will get to the FAILED is from a goto anyway, so why not just select the code, Ctrl+Shift+R, Extract Method...? (You aren't some kind of masochist who develops C# without ReSharper, right?) One big advantage is that with a function, you not only know that every function call will send the program to it, but you also know that the only way execution can end up inside the function body is if someone made a method call.

This may seem subtle, but suppose for instance you carelessly deleted the return query; statement in your example. The IDE will not warn you about function has no return statement and the program will happily run right through, auto-failing every time. There you have your mysterious bug. Amusing now, perhaps, but not so amusing 2 years later when you discover this happening after spending a day debugging.

Maybe you are smart enough to not make such mistakes, but the IDE isn't. IMO one of the big reasons for using C# at all is that it plays nice with the IDE and allows for powerful refactoring and automated code analysis (otherwise just use Python). If your code is full of gotos, you are effectively negating a major advantage of the language you chose.

Much like how you wouldn't make everything an object (you could, but why would you when you can just use C#'s nice type safety?), goto isn't the end of the world, but in C# specifically, there is almost always a better alternative. These alternatives include (please add to the list if I missed any):

• continue
• break
• Functions/methods
• throw
• try/catch
• switch

Answer 5

You could stick with the original slightly modified: There are 3 possible outcomes

• query is unaltered
• query is altered by query.OrderBy(defaultSort)

• query is altered by query.OrderBy(String.Format("{0} {1}", field, direction))

  public static IQueryable<T> Sort<T>(this IQueryable<T> query, String field, String direction, Expression<Func<T, Int32?>> defaultSort = null)
  {
  
      var result=query;
  
      if (defaultSort != null)
      {
          result = query.OrderBy(defaultSort);
      }
  
      try
      {
          result = 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.
      }
  
      return result;
  
   }
  

  No need for GOTO

Answer 6

Your comments are inconsistent with the code. A more accurate comment (if necessary) would be // Field wasn't found; try the default sort if supplied.

You have fixated on the use of goto, but ignored the other evil practice in your code, which is the single point of return. The guideline that there should only be one return statement in a function is more applicable to C, where you might need ensure that some cleanup code gets executed before returning. Not surprisingly, if you use C-style returns, then you also tend to end up with C-style gotos.

Get rid of the single point of return, and the goto problem also goes away.

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)
    {
    }
    catch (ParseException)
    {
    }

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

Answer 7

Perhaps interesting is the fact that almost this exact usage of goto is used quite a bit in the Linux Kernel (as shown here), so this is a somewhat valid use.

I think more important here, though, is that C# has much better error handling mechanisms than C and they should be used instead, which the previous answers have clearly shown.

Here mentions this usage in C vs C++.