-
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?
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
After the messagebox can't we just do a Exit sub?
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
Hmmm I never thought about it that way. Good to know of that angle.
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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 ?
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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..
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
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.... ;)
-
Re: Good Programming practice- using goto
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..
-
Re: Good Programming practice- using goto
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...
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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.
-
Re: Good Programming practice- using goto
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
-
Re: Good Programming practice- using goto
Usually, more than one try for the DB. One for the connection, and then one outside the main loop. Nested