tags:

views:

1707

answers:

14

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

+10  A: 

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
Joel Coehoorn
that's great. But do you have an alternative?
Micah
Gimmee a minute: I was formatting your code to be readable ;)
Joel Coehoorn
I would agree with that statement.
Kevin
thank you! I'ts been one of those days!!
Micah
@Joel Coehoorn: Unfortunately, this suffers from the fact that it will leave the connection in an indeterminate state in the face of an exception. If the exception occurs BEFORE the call to Commit or Close then you will leave the connection open and the resource locked.
casperOne
@Joel Coehoorn: Because of this, a class that implements IDisposable to track the connection should be used to close the connection. Also, the same could be done for the transaction, but it would be easier to just use TransactionScope instead.
casperOne
Agreed on the need for try/finally or using. One step at a time.
Joel Coehoorn
+1  A: 

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.

Kevin
+6  A: 

Really bad design. Yes.

Jason Punyon
+2  A: 

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.

casperOne
+1  A: 

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.

JohnFx
A: 

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

Allen
A: 

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.

tvanfosson
>Pfft< 20 Years. I've Programmed my ENTIRE LIFE without using it :)
Jason Punyon
What is this g...o...t...o everyone is talking about?
EBGreen
Never programmed in Fortran, eh?
tvanfosson
Its like a GOSUB without the return trip. =)
JohnFx
A: 

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.

hmcclungiii
+2  A: 

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();
    }    
}
Orion Edwards
I would not put a return statement in the middle of a method like that. If you want to extend the method so it does more stuff, more stuff will not get executed when something bad happened.
Renze de Waal
... that's the whole point. If something bad happens you DONT want to execute more stuff.
Orion Edwards
Early exits are just fine. Many times they can make the code much easier to understand.
Matthew Whited
A: 

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.

vanslly
+3  A: 

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.

Jon B
A: 

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.

Renze de Waal
A: 

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!

A: 

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

Bill K