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

• rolfl: Obligatory: xkcd.com/292
• Flambino: I'm reminded of Apple's infamous goto fail bug/security flaw. Granted, the problem was really caused by missing braces and likely a bad merge. But still, goto fail is almost what you wrote here :)
• Flambino: @Quentin Oh, sure, the code's got problems regardless. I just thought the similarity in goto fail/goto FAILED was funny
• Ross Millikan: Numerical Recipes had a great essay (decades ago) on structured code. They made the point that structured code made it easy to understand the control flow. The problem is not the goto-you know where it goes. The problem is the label. When you see one, you wonder "where can I come from to get here?"
• CodesInChaos: Can you include the source of OrderBy? This is not the BCL version of it.

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

• CB Bailey: I think that this is the simplest and clearest restructuring of the code. I'm not a C# expert but I'm assuming that rebinding the local query has no subtle side effects and that in this function query = query.OrderBy(...); return query; has the same effect as return query.OrderBy(...);. If so, I'd make the same simplification and do if (defaultSort != null) { return query.OrderBy(defaultSort); }.
• 5gon12eder: @CharlesBailey Fair point. It could even be reduced to return (defaultSort != null) ? query.OrderBy(defaultSort) : query;. But it's too late now that I'd feel comfortably editing my answer. Btw, I'm not a C# expert either.

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

• Superbest: In this sense, goto does seem to have an advantage: Functions must be passed parameters before they can use them. For instance, if his FAILED block was manipulating 20 different variables, the function signature would be very cumbersome... Although something like that is probably a symptom of bad design in the first place.
• rolfl: While you are correct, @Superbest - it is also something that compilers/optimizers are very efficient at optimizing (inlining), and although "premature optimization" is often thrown around as an excuse for all sorts of things, in this case, the GOTO would be premature as an optimization.
• Quentin: @rolfl I think his point is less about worrying for the compiler, and more aboute dragging a function with a dozen parameters around...

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

• Mathieu Guindon: I would upvote this answer, but "The IDE will not warn you about function has no return statement and the program will happily run right through" - CS0161 says otherwise...
• Superbest: @Mat'sMug There are multiple return statements in the example, I didn't mean the last one.

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

• rolfl: This results in a double-ordered query when the try-block succeeds, and is semantically different to the OP's code... is this intended?
• Thomas Junk: Could you elaborate? I assumed a simple reordering in case the default is non-null and the tryblock succeeds.
• rolfl: Can we take this to chat? The 2nd Monitor

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++.

• Roy T.: I think kernel code is almost always a bad example for best practices. When you're working on such a low level other rules and constraints apply :).
• Clukester: @RoyT. I agree, and I'm definitely not claiming any best practices for C or even for kernel code. I just wanted to provide some context from the broader C* family.