When is it OK to use the GoTo statement in VB.Net?

Question

I have process that needs to create a bunch of records in the database and roll everything back if anything goes wrong. What I want to do is this:

Public Structure Result
    Public Success as Boolean
    Public Message as String
End Structure

Private _Repository as IEntityRepository

Public Function SaveOrganization( _
    ByVal organization As rv_o_Organization) As Result
    Dim result = Result.Empty

    _Repository.Connection.Open()
    _Repository.Transaction = _Repository.Connection.BeginTransaction()

    ''//Performs validation then saves it to the database
    ''// using the current transaction
    result = SaveMasterOrganization(organization.MasterOrganization)
    If (Not result.Success) Then
        GoTo somethingBadHappenedButNotAnException
    End If

    ''//Performs validation then saves it to the database
    ''//using the current transaction
    result = SaveOrganziation(dbOrg, organization)
    If (Not result.Success) Then GoTo somethingBadHappenedButNotAnException

somethingBadHappenedButNotAnException:
    _Repository.Transaction.Commit()
    _Repository.Connection.Close()
    Return result
End Sub

Is this an ok use of the GoTo statement, or just really bad design? Is there a more elegant solution? Hopefully this sample is able to get the point across

Answer 1

If you have to ask, don't do it.

For your specific code, you could do it like this:

Public Function SaveOrganization(ByVal organization As rv_o_Organization) As Result
    Dim result As Result = Result.Empty

    _Repository.Connection.Open()
    _Repository.Transaction = _Repository.Connection.BeginTransaction()

    'Performs validation then saves it to the database 
    'using the current transaction
    result = SaveMasterOrganization(organization.MasterOrganization)

    'Performs validation then saves it to the database 
    'using the current transaction
    If result.Success Then result = SaveOrganziation(dbOrg, organization)

    _Repository.Transaction.Commit()
    _Repository.Connection.Close()
    Return result
End Sub

Answer 2

Really bad design. Yes.

Answer 3

Goto has such a terrible reputation that it will cause other developers to instantly think poorly of your code. Even if you can demonstrate that using a goto was the best design choice - you'll have to explain it again and again to anyone who sees your code.

For the sake of your own reputation, just don't do it.

Answer 4

There might be some extreme edge cases where it is applicable, but almost unequivocally, no, do not use it.

In this specific case, you should be using the Using statement to handle this in a better manner. Generally, you would create a class which would implement IDisposable (or use one that already does) and then handle cleanup in the Dispose method. In this case, you would close the connection to the database (apparently, it is reopened again from your design).

Also, I would suggest using the TransactionScope class here as well, you can use that to scope your transaction and then commit it, as well as auto-abort in the face of exceptions.

Answer 5

The only time you should use a goto, is when there is no other alternative.

The only way to find out if there are no other alternatives is to try them all.

In your particular example, you should use try...finally instead, like this (sorry, I only know C#)

void DoStuff()
{
    Connection connection = new Connection();
    try
    {
        connection.Open()
        if( SomethingBadHappened )
            return;
    }
    finally
    {
        connection.Close();
    }    
}

Answer 6

I would say extremely sparingly. Anytime I have had to think about using a GOTO statement, I try and refactor the code. The only exception I can think of was in vb with the statement On Error Goto.

Answer 7

There is nothing inherently wrong with goto, but this isn't a very ideal use of it. I think you are putting too fine a point on the definition of an exception.

Just throw a custom exception and put your rollback code in there. I would assume you would also want to rollback anyway if a REAL exception occured, so you get double duty out of it that way too.

Answer 8

Gotos are simply an implementation detail. A try/catch is much like a goto (an inter-stack goto at that!) A while loop (or any construct) can be written with gotos if you want. Break and early return statements are the most thinly disguised gotos of them all--they are blatant(and some people dislike them because of the similarity)

So technically there is nothing really WRONG with them, but they do make for more difficult code. When you use the looping structures, you are bound to the area of your braces. There is no wondering where you are actually going to, searching or criss-crossing.

On top of that, they have a REALLY BAD rep. If you do decide to use one, even in the best of possible cases, you'll have to defend your decision against everyone who ever reads your code--and many of those people you'll be defending against won't have the capability to make the judgment call themselves, so you're encouraging bad code all around.

One solution for your case might be to use the fact that an early return is the same as a goto (ps. Worst psuedocode ever):

dbMethod() {
    start transaction
    if(doWriteWorks())
        end Transaction success
    else
        rollback transaction
}
doWriteWorks() {
    validate crap
    try Write crap
    if Fail
        return false
    validate other crap
    try Write other crap
    if Fail
        return false
    return true
}

I think this pattern would work in VB, but I haven't used it since VB 3 (around the time MS bought it) so if transactions are somehow bound to the executing method context or something, then I dunno. I know MS tends to bind the database very closely to the structure of the code or else I wouldn't even consider the possibility of this not working...

Answer 9

I use goto's all the time in particular places, for example right above a Try Catch, in case a you prompt the user, "Retry?, Cancel", if retry then Goto StartMyTask: and increment the Current Try of Maximum retries depending on the scenario.

They're also handy in for each loops.

For each Items in MyList

 If VaidationCheck1(Item) = false then goto SkipLine
 If ValidationCheck(Item) = false then goto skipline

 'Do some logic here, that can be avoided by skipping it to get better performance.
 'I use then like short circuit operands, why evaluate more than you actually have to?

 SkipLine:
Next

I wouldn't substitute them for functions and make really huge blocks of long code, just in little places where they can really help, mostly to skip over things.

Answer 10

Everytime I've seen a goto used, simple refactoring could have handle it. I'd reccomend never using it unless you "know" that you have to use it

Answer 11

I'm tempted to say never, but I suppose there is always one case where it may be the best solution. However, I've programmed for the last 20 years or so without using a Goto statement and can't foresee needing one any time soon.

Answer 12

Why not wrap each function call in a try catch block and when that is done, if one of your exceptions are thrown, you can catch it and close the connection. This way, you avoid the GOTO statement altogether.

In short, the GOTO statement is NOT a good thing, except in unusual situations, and even then it is usually a matter of refactoring to avoid it. Don't forget, it is a leftover from early languages, in this case BASIC.

Answer 13

I'd say use very sparingly as it's generally associated with introducing spagetti code. Try using methods instead of Labels.

A good case I think for using GOTO is to create a flow through select which is available in C# but not VB.

Answer 14

The go to statement has a tendency to make the program flow difficult to understand. I cannot remember using it during the last ten years, except in visual basic 6 in combination with "on error".

Your use of the go to is acceptable as far as I am concerned, because the program flow is very clear. I don't think using try ... catch would improve things much, because you would need to throw the exceptions on the locations where the go tos are now.

The formatting, however, is not very appealing:-)

I would change the name of the go to label to something else, because this location is also reached when everything is successful. clean_up: would be nice.

Answer 15

Eh, there was a decent use for it in VBscript/ASP for handling errors. We used it to return error handling back to ASP once we were done using on error resume next.

in .net? Heavens, no!

Answer 16

Everyone says avoid it but why. The GOTO syntax is a jump statement in assembly - very efficient.

Main reason to avoid it is code readability. You need to find the GOTO label in the code which is hard to eyeball.

Some people think that it might cause memory leaks but I've seen that experts say that this is not true in .NET.