Why this 'goto' statement usage is not recommended?

Question

We're having a big debate in the company whether or not goto statements should be used at all in projects. I personally find it to be adding clarity for the following scenario where I need to retry a web service call.

const string primaryWebServiceUrl = "https://example.com/Server.asmx";
const string secondaryWebServiceUrl = "https://example2.com/Server.asmx";

using (var ws = new Server())
{
    ws.Url = primaryWebServiceUrl;

start:
    try
    {
        wsAction?.Invoke(ws);
    }
    catch
    {
        if (ws.Url == secondaryWebServiceUrl)
            throw;

        ws.Url = secondaryWebServiceUrl;
        goto start;
    }
}

I believe that adding a loop in this case sacrifices code clarity and I find it to be an overkill to reference Polly just for having Retry logic.

Edit: Since everyone is saying that it's not recommended to use a goto statement here I'd like to learn more about why this is not recommended and what detrimental effects it can have. In my opinion this adds clarity but I can understand that the goto statement unwinding effect can be negative if not used correctly but in the example provided above, why the goto approach is not recommended?

Using `goto` is not recommended nowadays, and you can consider it as a bad practice! — Salah Akbari, Nov 20 '18 at 12:03
Since it's a feature available in the C# language and I find it to be more helpful than detrimental I don't see why not to use it unless you have a better argument. — Paul, Nov 20 '18 at 12:05
This is a wrong way of re-trying. rather consider using some sort or message queue and push the call back to the queue which probably can be taken up some other worker at some point for re-try — Rahul, Nov 20 '18 at 12:06
This doesn't add clarity, nor does it implement a retry. A `while` loop with a retry counter would do that nicely. To clean up this code you could *remove* `?.Invoke(ws)`, validate that the lambda exists right after it's passed as a parameter, use the Task-based async methods if available. If you want to implement a generic retry mechanism try Polly — Panagiotis Kanavos, Nov 20 '18 at 12:06
@Paul Have a look at this https://stackoverflow.com/questions/11906056/goto-is-this-bad — Salah Akbari, Nov 20 '18 at 12:06
That goto is not very detrimental to code readability. Which is the only thing a team should ever have heated debates about. Make it a constructive debate by offering a better alternative. `for(;;)` + `break` in my book, but that has some odds for firing off a debate as well :) — Hans Passant, Nov 20 '18 at 12:12

Dmitry Bychenko · Accepted Answer · 2018-11-20T12:42:08.793

2

It's valid, but not recommended; a more readable implementation is something like this

using (var ws = new Server()) {
  ws.Url = primaryWebServiceUrl;

  // Keep doing ...
  while (true) {
    try {
      wsAction?.Invoke(ws);

      // ...until Invoke succeeds
      break; 
    }
    catch { //TODO: put expected Exception type here
      // Something is very wrong; rethrow the exception and leave the routine 
      if (ws.Url == secondaryWebServiceUrl)
        throw;

      ws.Url = secondaryWebServiceUrl;
    } 
  }
}

Or even better (especially if we want to have many urls) - thank to Panagiotis Kanavos for the idea:

 string[] urls = new string[] {
   "https://example.com/Server.asmx",
   "https://example2.com/Server.asmx",
   "https://example3.com/Server.asmx", 
    ...
   "https://example123.com/Server.asmx", 
 };

 using (var ws = new Server()) {
   // Try each url from urls...
   for (int i = 0; i < urls.Length; ++i) {
     try {
       ws.Url = urls[i];
       wsAction?.Invoke(ws);

       // ... until success 
       break;  
     }
     catch {
       // The last url failed; rethrow the error
       if (i >= urls.Length - 1)
         throw; 
     }  
   } 
 }

edited Nov 20 '18 at 12:42

answered Nov 20 '18 at 12:07

Dmitry Bychenko

180,369
20
160
215

1

Or loop over the URLs. There are a lot of ways this code could be cleaned up without using `goto` – Panagiotis Kanavos Nov 20 '18 at 12:09
why not use a boolean that is declared outside the loop, set it to false at the beginning of the loop and change it to true if you run into the catch block? – Steffen Winkler Nov 20 '18 at 12:10
I personally find a loop to be exactly the same it's just that you have to pay attention at the loop condition and the possible exist cases. For my example I have two possible exit cases, the loop has 3 (including the "true" condition) exit cases to evaluate. – Paul Nov 20 '18 at 12:11
Or a `while (retryCount > 0)` would justify the loop over the goto even better. – György Kőszeg Nov 20 '18 at 12:12
Can you detail why it's not recommended? – Paul Nov 20 '18 at 12:12
@Paul because it can lead to unreadable and unmaintainable code. The spaghetti effect. Imagine a project with ~5000 lines of code and some label/goto, good luck for debugging that – Cid Nov 20 '18 at 12:14
1

I think the main reason why it's not recommended because people still remember in horror how terrible it was to keep track of what the code is doing in vb6 (and even pascal) with all the `goto` creeping around in the code. If you have too many of them, debugging becomes a nightmare. If you want to only have one, you might find yourself in a slippery slope (Kinda like cigarettes...) – Zohar Peled Nov 20 '18 at 12:15
I am asking why it's not recommended for the particular case in my question. – Paul Nov 20 '18 at 12:18
Well, consider adding a new URL - so now you have a 2 way fallback - try with the first, then with the second, then with the third and throw only if all urls have been tried. This would make the use of a loop far easier - you can use a for loop and in the condition for `throw` just use the loop counter compared to the count of the array of urls -1. – Zohar Peled Nov 20 '18 at 12:33

score 0 · Answer 2 · answered Nov 20 '18 at 12:06

0

goto is considered really bad practice anywhere outside a switch statement. There are many much better constructs.

You could move your try/catch logic to a method and have a loop checking for the result (could be true/false) and then keep calling the method - instead of using goto

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/goto

This link explicitly mentions switch and getting out of deeply nested loops. Neither of those cases apply here.

answered Nov 20 '18 at 12:06

Per Hornshøj-Schierbeck

15,097
21
80
101

Those are common use cases not specific instructions telling people that the feature was intended only for those particular cases. – Paul Nov 20 '18 at 12:10
2

It's very uncommon to find goto used in modern code projects and when you do, they are almost always used either in a switch or to get out of a deeply nested loop. Most people would agree that the use of goto is distruptive and breaks readability and flow – Per Hornshøj-Schierbeck Nov 20 '18 at 12:17

Why this 'goto' statement usage is not recommended?

2 Answers2