Good Programming practice- using goto

OK, I know using GOTO is ugly old programming, but what is a better solution?

I give my user an OpenFileDialog and allow them to choose a file.
If I don't like the file they have chosen I give them a message explaining why and send them back using GOTO to choose again.

What would be a more elegant way of doing this?

Select Case, Try Catch with a flag, and when absolutely necessary, one GOTO statement isn't really bad.

What made them bad, was using them to control the execution of the program. There could be 5 in one sub, just to get out of the sub.

Following around the logic was known as 'spaghetti code' for that reason

I'd go for a loop with a flag and select case... the problem I have with goto is that "what harm can one little goto do?" ends up being repeated several times, and can lead to hard to follow code (good old fashioned spaghetti code). If used minimally, and properly, there's generally no problem with it. Unfortunately developers tend to lack restraint when using something so simple.

-tg

edit - yes, I'm guilty of having writ my share of spaghetti code in the past. :D

I had a DrawPoker game in Quick Basic, which worked, but didn't use many functions or subs that didn't have a GOTO it there. Usually just to jump to the end of the module, or call a model form.

For the most part Goto should only be used as part of the On Error statement. (VB6)
Loops, sub routines, functions are better choices for other code.

That said an occasional Goto is not a show stopper.

So long as the execution flow is low or not critical GoTo would be okay, although I don't use it at all, since you are looping until a condition is met, so why not use Do, or For loops.

However, I believe you are looking for the way to cancel the ok button.
Essentially right?
With a visual openfiledialog control on the form:

Code:

---

    Private Sub OpenFileDialog1_FileOk(ByVal sender As System.Object, ByVal e As System.ComponentModel.CancelEventArgs) Handles OpenFileDialog1.FileOk
      'If file is not good(this test app)
        If IO.Path.GetFileName(OpenFileDialog1.FileName) = "WindowsApplication1.exe" Then
            e.Cancel = True
              MessageBox.Show("Can't point to this file for example")
        End If
    End Sub

---

After the messagebox can't we just do a Exit sub?

you could .... *I* don't... I subscribe to the Single-Entry, Single-Exit design when it comes to Subs/Methods/Functions. Meaning there's one point of entry, and one place, and one place only, where the s/m/f exits, and Exit Sub violates that. That's not to say using Exit Sub is bad, I just tend to avoid it if I can.

-tg

Hmmm I never thought about it that way. Good to know of that angle.

Yep, I do not like to use exit sub much either.
In an instance like we have here a simple loop would work fine. Something like

Code:

---

Do
    GetFileName
    If Filename Ok then
        Exit Do
    Else
        Message
    End If
Loop

---

Well here we figured out that there isn't a need for a loop in this request.

Wrong file, so cancel ok(open) in .NET way, inform user to try again.
Elegant.

While we're on the topic of "the good, the bad, and the ugly" coding practices, a thin that irritates em is when people make use of an empty catch block. Meaning, they have something like this :

Code:

---

Try
'Do stuff
Catch ex As Exception

End Try

---

What are your thoughts on this ?

Here is my view.

I've seen empty catches too, usually in examples where the coder may not have wanted to put in custom error handling in a generic example. It's up to you. I may have done it myself once or twice.

Although sometimes, it may be used like that to ignore a nasty framework error message, or some other undesirable chain of events.
While only if the message is worst than what happens if ignoring the error.
One should always be mindful of the worse case scenario, before ignoring an error.
EDIT: For example WaitForInputIdle API can fail with certain process handles and conditions(non graphic),
so Try/Catch/Ignore is a way to ommit the nasty hanging error, which was worse than not waiting... it didn't need to wait to begin with, and should have been ignored.
Although a full defensive technique, would check for graphical interface first before even trying.

Try Catch Ignore, behaves slightly different than On Error Resume Next ofcourse.
A Try Catch block can stubbornly lock up and refuse to try "ever" again, with certain various errors, until restarted.

Usually when I use On Error Resume Next, the code doesn't even need any error handling, which does cause some coders to snap to judgment, and frown on the rest of your work.
I may use it as intended with defensive styles, but usually to cover for unforseen errors, hardware errors, or solar flares etc. It's always placed last in those cases. I try not to catch an error late, in favor of defensive programming techniques, return values, file checking etc.

On the subject of Empty Try Catch blocks.. i often use it to check for the existence of a Device, or SQL.
Something like...

Code:

---

        Public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                Reply = True
                sqlConn.Close()
            Catch ex As Exception
                Reply = False
            End Try
            Return Reply
        End Function

---

Here i'm realy not interested in what the error was, Just that the SQL connection string did not work.

I've built similar functions to search for external USB or COM devices, and it helps to use these just before committing to some intricate function that may hang the system if the device is not responding..

Quote:

---

Here i'm realy not interested in what the error was, Just that the SQL connection string did not work.

---

you have missed return of any function.

Code:

---

public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                return True
                sqlConn.Close()
            Catch ex As Exception
              Return false
            End Try
            Return Reply

        End Function

---

I don't agree xtab :)

Here, a variable was set to either True or False, and that variable gets returned. By using the variable it is the same as saying Return True or Return False. It is even the same as saying TestConnection = True or TestConnection = False

technicalities.... ;)

Quote:

---

Originally Posted by xtab

you have missed return of any function.

---

Granted, I'll give you that.. but what about if it was written like this.. EDIT: I (mis)understood this statement as saying 'the Catch block still has code in it'

Code:

---

        Public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean = False
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                Reply = True
                sqlConn.Close()
            Catch ex As Exception
            End Try
            Return Reply
        End Function

---

This is also 'Good' programming, and here there is nothing in the catch block.. However the result is the same..

But now going back to the OP one of the better methods to use is something like this.

Code:

---

    Public Sub SelectAfile()
        Dim FileOK As Boolean = False
        Dim OpenCancel As Boolean = False
        Dim Result As DialogResult
        Do
            'Show Open File Dialog
            Result = OpenFileDialog1.ShowDialog()
            If Result = Windows.Forms.DialogResult.OK Or Result = Windows.Forms.DialogResult.Yes Then
                'Check if file satisfactory
                If OpenFileDialog1.FileName = "HappyFile.ext" Then
                    FileOK = True
                End If
            Else
                'User Cancled out
                OpenCancel = True
            End If
        Loop Until FileOK Or OpenCancel


        If FileOK Then
            'We can now process the file ...

        End If

    End Sub

---

there are no Nasty GOTO's and an easy to follow sub..

Quote:

---

Originally Posted by xtab

you have missed return of any function.

Code:

---

public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                return True
                sqlConn.Close()
            Catch ex As Exception
              Return false
            End Try
            Return Reply
        End Function

---

---

Let me also just add that writing it like this is 'VERY BAD' Practice...

'RETURN X' is the same as using 'EXIT FUNCTION' but sends the variable 'X' as the return value..
In this case if the connection did not fail, the connection would stay open as 'sqlConn.Close()' will never be called...

And like TechGnome put it.. I subscribe to the Single-Entry, Single-Exit design when it comes to Subs/Methods/Functions... This is solid advice...

Quote:

---

one of the better methods to use is something like this

---

Code:

---

      Do
            'Show Open File Dialog
            Result = OpenFileDialog1.ShowDialog()
            If Result = Windows.Forms.DialogResult.OK Or Result = Windows.Forms.DialogResult.Yes Then
                'Check if file satisfactory
                If OpenFileDialog1.FileName = "HappyFile.ext" Then
                    FileOK = True
                End If
            Else
                'User Cancled out
                OpenCancel = True
            End If
        Loop Until FileOK Or OpenCancel

---

Ooo, well one of but not really in the particular case of dialogs.
I must reiterate that the thread is already resolved with the most proper solution on post #6. It's seems that this is the second time the resolution was overlooked. No?

Actually dialogs don't like to be called in a loop, and can in fact wonk out on you with spurious results. I've had it happen in several projects, that is until I discovered the proper way to cancel the ok(open) button before it's processed.

Thus there is no need to repeat, Do over, or Go Back To, since you cancelled it from ever happening(but leaves the dialog still open!). If there were no ability to cancel, then a loop might be an easy route, but trust not it's stability.

Code:

---

public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                return True
                sqlConn.Close()
            Catch ex As Exception
              Return false
            End Try
            Return Reply
End Function

---

I certainly agree that is not even practice, nevermind good practice, however I think you can use Finally to close it?

I guess practice depends alot on the habit level of the individual, and if one trusts oneself to be careful enough. Forever changing as one grows.
I myself am very mindful about the paths of execution, and so I may tend to exit whenever I feel it's really ok, or shorter code etc. I can see the point of view of a "Single exit" strategist though. I do get it.

Quote:

---

Originally Posted by xtab

you have missed return of any function.

Code:

---

public Shared Function TestConnectionString(ByVal ConnectionString As String) As Boolean
            Dim Reply As Boolean
            Try
                Dim sqlConn As New SqlConnection(ConnectionString)
                sqlConn.Open()
                return True
                sqlConn.Close()
            Catch ex As Exception
              Return false
            End Try
            Return Reply

        End Function

---

---

Also agree this is incredibly bad as the sqlConn.Close() line would never get executed.

As a side-note to this, I have read that you should never close a database connection in a Finally block because if an exception is thrown in the Catch block, the Finally block will never execute.. ie:

Code:

---


        Dim cn As New SqlConnection(connectionString)
        Dim cmd As New SqlCommand
        Dim trans As SqlTransaction

            Try

                'start the transaction and save the batch details first

                cn.Open()
                trans = cn.BeginTransaction

                With cmd
                    .Connection = cn
                    .Transaction = trans
                    .CommandType = CommandType.StoredProcedure
                    .CommandText = "usp_Batches_Save"

                    With .Parameters
                        ' Add some parameters
                    End With

                    .ExecuteNonQuery()

                End With

                ' if there are orders associated with the batch then update the batch number of
                ' each order.

                If Not sBatch.Orders Is Nothing Then
                    For i = 0 To sBatch.Orders.Length - 1

                        cmd = New SqlCommand
                        With cmd
                            .Connection = cn
                            .Transaction = trans
                            .CommandType = CommandType.StoredProcedure
                            .CommandText = "usp_Batch_Order_Save"
                            .Parameters.Add("@link_id", sBatch.Orders(i))
                            .Parameters.Add("@batch_no", batchNo)
                            .ExecuteNonQuery()
                        End With
                        cmd = Nothing
                    Next
                End If

                trans.Commit()
                trans = Nothing
                cn.Close()

            Catch ex As Exception

                trans.Rollback()
                trans = Nothing

                cn.Close()

                Throw ex

            End Try

---

In the above code, the connection will always close if there is an error but if I had done this:

Code:

---

                trans.Commit()
                trans = Nothing

            Catch ex As Exception

                trans.Rollback()
                trans = Nothing

                Throw ex

            Finally
                cn.Close()

            End Try

---

and an error occured when the transaction was rolled back (for example) then the Finally block would never execute and the connection would not get closed.

Just something I read in a book and thought I'd share.

Thanks for sharing.

I wan't thinking of putting error prone code into the catch block, but you're probably right, if you did. Yet another reason why Try blows hard.
I meant, that with the simple example posted returning would be okay if you closed in the finally.

I just want to make sure that we are not saying that exiting with a return is bad practice. Not at all, that's what it's for, it's new, not old vb6-ish.
Once you know that it means exit with the return value, uhh it's pretty self explanatory, and you shouldn't be using it if you don't know what it does.
Thats why I don't understand "bad practice" logic, or at least it doesn't apply to me, since I would never exit before I had to do something final.
Falling rock behavior is common sense.

One of the reasons I advocate Single Entry, Single Exit practice is for maintainability. When there's a problem with a specific part of an application, if it's a function, I tend to go to the Return... breakpoint... find out what it's returning. If there's a single exit, it's easy to find, and I can trace it backwards from there. Plus six months from now when the off-shore developer has to dig through my code, it's consistent. In the end that's what it really is about, reliability and consistency.

I don't necessarily think Try blows... it just requires some creative thinking sometimes to get it to do what you want. So far, I haven't been in a situation where I've thought "Man, I could really use On Error GoTo right now" ... personally, I like Try...Catch ... it allows me to deal with my exceptions right where the happen and I don't need to build error handlers at the bottom, checking for err.number as I go. Having said that... OEGT is nice in that you can Resume Next... with a Try... not so easy... but like I said, it just requires some creative thinking sometimes.

-tg

Well I guess if you are using breakpoints, it would be easy to track to.
I just dont' use the debugger at all really, since it causes it's own set of errors. I avoid it completley, and my life is better for it. lol

I only noticed that try blows hard, because I code alot into my library, and most of it is pretty complex. When you get to this level, creativity(of which I have plenty) will get you no where with certain errors.
Plus, when there are several things to catch, the code gets bulky with 3 additional lines per try.

False Premise:
Try, will always continue to try a block of code, when called upon, until it succeeds.

Quote:

---

False Premise:
Try, will always continue to try a block of code, when called upon, until it succeeds.

---

Didn't realize that was a premise in the first place, let alone false.

Too bad they couldn't find a way to make ReTry work efficiently. That would have been a truly wonderful asset.

Then again, perhaps is a good thing there isn't a ReTry command, as I could very easily see an infinite loop happening because the database server failed.

Just so I'm clear, I don't mean the Try block should be expected to loop itself somehow, but I mean when the block is called upon repeatedly it might ignore the call to even try.

Quote:

---

see an infinite loop happening

---

Yeah, I thought of that too. It might be considered good (unique)for the Try block to lock up, preventing some kind of infinite loop in shotty code. As long as we know it might do that, and we can program accordingly without the expectation that it will fire always.

However, it appears that very few people encounter, or identify it without any documentation about the quirk. Some people may have run across it, but never figured out what was going on.

I think it is just an error, and not "by design", but please correct me if there is documentation on this.

I couldn't tell you if there is or not. I've never had a situation where the Catch didn't fire off in the event of an exception. Even in a loop. But then again we're not usually doing large high-speed loops. And more often than not, our loops are inside the Try. 9 times out of 10 they are database writes.

-tg

Usually, more than one try for the DB. One for the connection, and then one outside the main loop. Nested