Good Programming practice- using goto

[happyme]

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?

[dglienna]

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

[TechGnome]

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.

[dglienna]

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.

[DataMiser]

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.

[TT(n)]

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:

    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

[viperbyte]

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

[TechGnome]

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

[viperbyte]

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

[DataMiser]

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

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

[TT(n)]

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.

[HanneSThEGreaT]

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 :

Try
'Do stuff
Catch ex As Exception

End Try

What are your thoughts on this ?

[TT(n)]

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.

[GremlinSA]

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

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

[xtab]

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.

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