Acceptable use of GoTo?

Question

Hello all.

I'm currently rewriting an old VB6 program in C# in the .Net Framework 2.0 (not my choice, it was decided by the company). For the most part, things have gone pretty well. The program measures incoming data from a precision grinding machine and displays graphs and dials to display the accuracy.

The original programmer was a mechanical engineer, however, not a software engineer. The program works, but there's a bit of sloppy code here and there. Most notably, I've run into a few GoTo statements. It's been pretty easy to just stick things in a loop, where necessary, and get the same functionality out of it.

I've run up against a case in the original code, however, where it seems like the GoTo is doing more than just simulating a loop. It has a couple different exit conditions. It looks something like this (not the actual code, just something short I made up to demonstrate):

VB6 Code

Public Sub Tick()
    Dim condition1 As Boolean
    Dim condition2 As Boolean
    Dim testNumber As Integer

    beginning:    'The GoTo label'

    ' (... Some Other Code Here ...)'

    If condition1 = True Then
        goto beginning
    Else
        ' (... Do some calculation ...)'
    End If

    If condition2 = True Then
        ' (... Do some calculation ...)'
        goto beginning
    End If

    Select Case testNumber
        Case 1: '(... Some code ...)'
        Case 2: '(... Some code ...)'
        Case 3: '(... Some code ...)'
        Case 4: goto beginning
    End Select
End Sub

The actual code might have a few less conditions than that, but the basic idea is that there are a few different things that cause it to loop back on itself. Is there a good way to go about writing a loop for a situation like that, or is this a case in which a goto statement would be acceptable? (Admittedly, a non-goto solution would be preferred).

Thanks for your time and consideration.

Note: I tried using a while(true) loop with a break; statement, but it caused the program to get caught in an infinite loop and lock up. Would it be more advised to write a long while loop containing several conditions (with and/or, etc.)?

Answer 1

A while(true) loop should be fine, if you have a break at the end of it and continue wherever there was previously a goto. However, this should definitely only be the first step - it sounds like a vigorous refactoring is called for.

Answer 2

I'd wrap it in a unit test and fire various values through it and record what the results are.

Then as you refactor the code into C# you can use the results of the test to verify your actions.

Answer 3

Start by putting the body of that loop into a separate function, and replace the gotos with `return's -- or prehaps several separate functions :

If condition1 = True Then 
    goto beginning 
Else 
    ' (... Do some calculation ...)' 
End If 

should become

If not condition1
      DoSomeCalculation()
End If

Soon the logic on when to loop & when to exit will emerge. When that happens, refactoring this code should become as trivial as what you've already done.

Answer 4

While this case looks good enough for a do/while true loop I've seen some cases that aren't.

Outside of a lexer or other FSA mechanism, it is my judgement that more than one goto per 2000 lines means you're doing something wrong.

Of course, if you have a recurring idiom that has a goto that's another story as recurring idioms override style rules. Idiom = consistent, consistent = readable.

Answer 5

I think your first step should be extracting all the '(do some code)' into their own methods. Once you've done that, the actually code flow will become a little clearer.

Depending on how nested it is, there are a few possible ways to accomplish this (hard without actual code).

(I'm a C# coder, I don't know VB, forgiveness please)

Recursive

Public Sub Tick()
    Dim condition1 As Boolean
    Dim condition2 As Boolean
    Dim testNumber As Integer

    If basecase = True Then
       return;
    EndIf

    ExecuteInitialzerStuff();

    If intialized = False Then
        Tick();
        return;
    Else
        ExecuteAffirmationStuff();
    End If

    If affirmed = True Then
        ExecutePostAffirm();
        Tick();
        return;
    End If

    Select Case testNumber
        Case 4: Tick();
    End Select
End Sub

another option would be to break each option into a discrete code flow

Public Sub Tick()
    Dim condition1 As Boolean
    Dim condition2 As Boolean
    Dim testNumber As Integer

    If condition1 = true Then
       Tick_Condition1();
       return;
    EndIf

    If condition2 = true Then
       Tick_Condition2();
       return;
    EndIf

    Tick_Switch(testNumber);

Once you've broken down each of the separate tasks that each code section is trying to accomplish, it should probably become clear that this method should be deleted entirely, and split up into a few separate Tick() methods, each called TickInit() TickDestroy(), TickSkyFalling(); or whatever, depending on the situation.

I believe that trying to refactor this function in place is the wrong decision. But I can't be certain without seeing the actual code.

Answer 6

In a switch-statement:

        switch (groupMembershipStatus)
        {
            case SocialGroupMembershipStatus.Banned:
                return redirect();
            case SocialGroupMembershipStatus.MembershipRequestDenied:
                Abc();
                goto case SocialGroupMembershipStatus.Banned;
        }

(As you can see I just wrote a goto in production code and I was wondering if there is a C# question about this use of goto!)