PDA

View Full Version : [SOLVED] Acceptable to use Goto



fredlo2010
03-17-2014, 04:07 PM
Hello guys,

Ok, since I started learning VBA I have read that using Goto is a bad practice and that should be avoid at all cost. And I have done that, actually I have never used this unless I need to drive the code because of an error.

Now I have a very complex code and I need to check for several stuff before the actual code runs: completeness, resources... is it ok to use Goto to direct the code to the Exit_handler? I should also add that not all the checks will fail because of errors. So I could not lets say leave the inner Sub without error handling and wait for it to be caught by the outer.

This is my code:


Sub MacroHome()

Application.ScreenUpdating = False
Application.EnableEvents = False

' Check that all the values in the home page have been entered correcly.
' If it comes back clean the macro should continue normally.
If Not HomePageDataOk Then GoTo Exit_Handler

' Check if the resource file and all its elements are present and ready to
' be used.
' A varibale is used to check the status of the errors. The user has the option
' to continue.
Call ResourceCheck
If g_blnExit = True Then GoTo Exit_Handler


' Check that all the Sheets necessary for the macro to run are present:
If Not HasToolSheets Then GoTo Exit_Handler


'Initialize variables
Call Initialize
If g_blnExit = True Then GoTo Exit_Handler


' Check if the macro was run for today
If Not g_blnSameDayReport Then
' Call the populate if balances from Brokers.
Call AddBalances
End If


Exit_Handler:

' Finalize the report, select landing page and appropriate messages.
Call Finalize

' Reset the variable that keeps track of the sheets name so cannot be renamed.
' TODO: This part will start setting things for the migration to the Addin.
ThisWorkbook.g_strSheetName = ActiveSheet.Name

Application.ScreenUpdating = True
Application.EnableEvents = True

Exit Sub

End Sub

This is a sample of one of the checks:




Function HomePageDataOk() As Boolean

' Assertion that the data is ok
HomePageDataOk = True

' Check for a value in the combobox
If Home.cboName.Value = vbNullString Then
MsgBox "The Name is Empty.Please enter a Name to proceed.", _
vbInformation + vbOKOnly, "Invalid"
Home.cboName.Select
HomePageDataOk = False
Exit Function
End If


' Check if a valid date was entered
If Not IsDate(Home.Range("DATE").Value) Then
MsgBox "The date entered is invalid. Please enter a valid date to proceed.", _
vbInformation + vbOKOnly, "Invalid Date"
Home.Range("DATE").Select
HomePageDataOk = False
Exit Function
End If


' Check if the date entered is in the future.
If Date < DateValue(Home.Range("DATE").Value) Then
MsgBox "The date entered is in the future. Please enter a valid date to proceed.", _
vbInformation + vbOKOnly, "Invalid Date"
Home.Range("DATE").Select
HomePageDataOk = False
Exit Function
End If

End Function

Thanks a lot for the help

MWE
03-17-2014, 04:23 PM
Although many will argue that the use of GoTo statements are not necessary and lead code that is difficult to understand, I disagree. There are lots of times with a simple GoTo is much easier to implement and understand that other approaches. Your example of an exit to an error handling code block is a good example.

SamT
03-17-2014, 06:57 PM
GoTo has become a good programming tool. To understand why it carries so much residual angst, imagine writing your entire process, including HomePageDataOK, AddBalances, Initialize, and HasToolSheet all in one procedure using only If...Then...End If and GoTo Line#, (No Elses or ElseIFs.)

And that's the way it was. As new tools became available the Jeune Ecole fought the old dinosaurs over their continued and habitual use of GoTo because there might be hundreds of GoTo Line#'s in a thousands of lines procedure.

A simple Loop


i = 1
j = 1
i = i + 1
j = j + 1
X = i * j
Print X
If j < 5 GoTo #4
If i < 5 GoTo #3

fredlo2010
03-17-2014, 07:07 PM
Thank you guys for all the replies. I was a little scared I was using too many GoTo statements. But it makes sense in this context.

Regards

SamT
03-17-2014, 07:13 PM
yer 2 fast

fredlo2010
03-17-2014, 07:26 PM
Really? Looping like this ? I could have never imagined. I totally get the warnings about the use of GoTo.

Thanks for the info SamT. I am going to look this up right now.

snb
03-18-2014, 03:35 AM
You'd better check in the events of the userformcontrols:
If values are correct the commandbutton will be shown, otherwise it won't.
(PS. I don't know what Home.Range("DATE").Value is : a range in a worksheet ?)


Private Sub cboName_change()
commandbutton1.visible= (cboname<>"")*(isdate(Home.Range("DATE").Value)*(Date < cDate(Home.Range("DATE").Value))
End Sub

No need to use any goto, because you do the checking while the user is 'inputting'
You also won't have to bother users with messages they did something 'wrong'.

Bob Phillips
03-18-2014, 05:23 AM
I personally am not dogmatic when it comes to GoTos; the usual riposte is that GoTos create spaghetti code, but I always answer that spaghetti programmers create spaghetti code, not GoTos. I have seen code that is pure spaghetti without a single GoTo in sight. But, having said that, I try to avoid using them (error handling excepted) because I feel the code looks better, reads better if there are no GoTos, and usually there is a better way of handling it. If I am worried that my code is nesting too much, what I tend to do is to raise a pseudo-error and handle that error specifically in my error handler.

fredlo2010
03-18-2014, 06:11 AM
You'd better check in the events of the userformcontrols:

Hi snb,

Always very helpful. Wow that's a very nice way to wrap up and condense my code lol. I cannot use the event for the Combobox because it will always be visible, this is the main entry of the macro. So users will Select and value, enter a date and press the run button.

Also this code will very soon detach from the workbook and become and add in so I want to keep the events to a minimum. I think it will get more complicated with events.

Yes "Date" is a range in the sheet.


Thanks a lot for the answer :)

fredlo2010
03-18-2014, 06:17 AM
Xld,

so would you say that something like the sample bellow will be a better option to my MacroHome()


Sub MacroHome()
Application.ScreenUpdating = False
Application.EnableEvents = False

' Check that all the values in the home page have been entered correcly.
' If it comes back clean the macro should continue normally.
If HomePageDataOk Then

' Check if the resource file and all its elements are present and ready to
' be used.
' A varibale is used to check the status of the errors. The user has the option
' to continue.
Call ResourceCheck
If Not g_blnExit Then

' Check that all the Sheets necessary for the macro to run are present:
If HasToolSheets Then

'Initialize variables
Call Initialize
If g_blnExit Then

' Check if the macro was run for today
If Not g_blnSameDayReport Then
' Call the populate if balances.
Call AddBalances
End If
End If ' Close checking initialization
End If ' Close checking ToolSheets
End If ' Close checking Resources
End If ' Close checking HomeSheet

Exit_Handler:

' Finalize the report, select landing page and appropriate messages.
Call Finalize

' Reset the variable that keeps track of the sheets name so cannot be renamed.
' TODO: This part will start setting things for the migration to the Addin.
ThisWorkbook.g_strSheetName = ActiveSheet.Name

Application.ScreenUpdating = True
Application.EnableEvents = True

Exit Sub

End Sub




or this less commented version:


Sub MacroHome()
Application.ScreenUpdating = False
Application.EnableEvents = False

' Check that all the values in the home page have been entered correcly.
' If it comes back clean the macro should continue normally.
If HomePageDataOk Then

' Check if the resource file and all its elements are present and ready to
' be used.
Call ResourceCheck
If Not g_blnExit Then

' Check that all the Sheets necessary for the macro to run are present:
If HasToolSheets Then

'Initialize variables
Call Initialize
If g_blnExit Then

' Check if the macro was run for today
If Not g_blnSameDayReport Then
' Call the populate if balances.
Call AddBalances
End If
End If
End If
End If
End If

Exit_Handler:

' Finalize the report, select landing page and appropriate messages.
Call Finalize

' Reset the variable that keeps track of the sheets name so cannot be renamed.
' TODO: This part will start setting things for the migration to the Addin.
ThisWorkbook.g_strSheetName = ActiveSheet.Name

Application.ScreenUpdating = True
Application.EnableEvents = True

Exit Sub

End Sub

Bob Phillips
03-18-2014, 06:34 AM
I think so, it is very clear that there is a dependency in those tests and it is easily traversed.

BTW, when I first started coding, GoTos were an accepted method of controlling flow. My first recollection of the railing against them, that has now turned into dogma, was when OOP first came in, it was very easy to get into a mess with this model, but it has evolved now, coding practices have improved, control structures have improved, so it is not so clear-cut in my view (although I still say avoid them if you can :)).

fredlo2010
03-18-2014, 07:44 AM
I guess a lot of my issues come from experience. I was thinking on a way of get rid of the Goto for quite sometime. And
If I am worried that my code is nesting too much [..] brought the light into my problem lol

Thanks a lot guys.

I am so happy I have learned something new today.

SamT
03-18-2014, 10:09 AM
Contrary to XLD, I think that one line IFs and GoTos or Exit Subs are a good thing for simple tests, because they make the code smaller and easier to comprehend.

Also note the indicated line in this version. To me, it has a bad-code smell.


Sub MacroHome()
Application.ScreenUpdating = False
Application.EnableEvents = False

' Check that all the values in the home page have been entered correcly.
' If it comes back clean the macro should continue normally.
If Not HomePageDataOk Then GoTo ExitHandler

' Check if the resource file and all its elements are present and ready to
' be used.
Call ResourceCheck
If g_blnExit Then GoTo ExitHandler

' Check that all the Sheets necessary for the macro to run are present:
If Not HasToolSheets Then GoTo ExitHandler

'Initialize variables
Call Initialize
If Not g_blnExit Then GoTo ExitHandler '<------------------------------------------------

' Check if the macro was run for today
If Not g_blnSameDayReport Then Call AddBalances
' Call the populate if balances.

Exit_Handler:
'
'
End Sub

It is really obvious in my VBE, because I used the tools Menu to change the colors

snb
03-18-2014, 10:26 AM
Sub MacroHome()
Application.ScreenUpdating = False
Application.EnableEvents = False

If HomePageDataOk Then ResourceCheck
If not g_blnExit and HasToolSheets Then Initialize
If g_blnExit and Not g_blnSameDayReport Then AddBalances

Application.ScreenUpdating = true
Application.EnableEvents = true
End Sub

Bob Phillips
03-18-2014, 11:41 AM
I personally have never striven to make my code smaller, therein lies madness in my view, but easier to read yes, and also more logical. That is why I think nesting is better, it is more logical in the context of the code.

Even if you do go for the GoTos, this is bad code in my view


Call Initialize
If Not g_blnExit Then Goto ExitHandler

Rather than set a global variable in Initialize and test it in the caller, better to make Initialize a function and return success or failure as the function result and test that


If Not Call Initialize Then Goto ExitHandler

or even raise a pseudo-error as I mentioned above


If Not Call Initialize Then Err.Raise Errors.TidyUp '

fredlo2010
03-18-2014, 12:20 PM
Thanks a lot for all the help guys. I really appreciate it.

@SamT
You are right this line is incorrect. This was my mistake while preparing the sample to post here.

If Not g_blnExit Then Goto ExitHandler '<------------------------------------------------

@snb Thats a very nice code. It sums up the logic.

@Xld
The reason I have a global variable is because some of those procedures have other callers inside. The worst is a report that has about 11 conditions nested and wrapped around several loops. Right in the middle of all that I have my global exit variable. So if anything goes wrong the procedure will exit and it would be better. In actuality I could leave this out but the end result is a blinking screen and the creation of unnecessary sheets.

And I though that If I used it once then I could use it in other places and keep it as a standard. Maybe I should go with the error for this particular session.

Thanks a lot guys :)

Bob Phillips
03-18-2014, 12:51 PM
I don't think you 'should' go with any of them particularly, just be aware of your options.

Paul_Hossler
03-18-2014, 02:55 PM
I personally have never striven to make my code smaller, therein lies madness in my view, but easier to read yes, and also more logical.


After all it's the person who had to read the source lines, not the computer


I've tried to be carefully judicious in my use of GoTo's, and over the past years (OK, too many years) I've found that if I'm careful and the destination is 'close' and I don't jump out of control blocks, it can simplify the code, and make IMHO the logic more transparent


Instead if multiple nested IF's or a huge multi-line AND statement, I find this easier to follow, especially when I come back in 6-12 months



Option Explicit
Sub JustMyOpinion()
Dim rRow As Range

For Each rCell In ActiveSheet.Cells(1, 1).CurrentRegion.Rows
With rRow

If .Row = 1 Then GoTo NextRow

If .Cells(1).Value = "No" Then GoTo NextRow

If .Cells(4).Interior.Color = vbRed Then GoTo NextRow

.Cells(8).Interior = vbGreen

End With
NextRow:
Next

End Sub



Paul

Paul_Hossler
03-18-2014, 03:08 PM
@Xld
The reason I have a global variable is because some of those procedures have other callers inside. The worst is a report that has about 11 conditions nested and wrapped around several loops. Right in the middle of all that I have my global exit variable. So if anything goes wrong the procedure will exit and it would be better. In actuality I could leave this out but the end result is a blinking screen and the creation of unnecessary sheets.


I'd read up on the ErrObject and using Err.Raise to trap your unique error situations and handling them appropriately

It's got a lot of features


Also VBA has some other control features that you can investigate such as Do .. Loop that might avoid having to react to error conditions and a global error variable



Repeats a block of statements (http://www.vbaexpress.com/forum/ms-help://MS.EXCEL.DEV.14.1033/EXCEL.DEV/content/HV10383569.htm) while a condition
is True or until a condition becomes True.
Syntax
Do [{While | Until}
condition]
[statements]
[Exit
Do]
[statements]
Loop

Or, you can use this syntax:
Do
[statements]
[Exit
Do]
[statements]

Loop [{While | Until} condition]



Home work assignment :-)

Paul

fredlo2010
03-19-2014, 04:48 AM
Thanks a lot for the advise. I have never used Err.Rise before. I will take a look into that.

Bob Phillips
03-19-2014, 05:30 AM
I showed it to you back in post #15.

fredlo2010
03-19-2014, 06:01 AM
Yes Xld, I meant it as in general. You both showed it to me :)

Thanks a lot fo all the help. I really appreciate it.

Aflatoon
03-19-2014, 08:53 AM
And here is an example of how not to use GoTo: http://www.excelforum.com/excel-programming-vba-macros/997862-error-handling-doesnt-seem-to-work-new-post.html

Bob Phillips
03-19-2014, 09:22 AM
That's an example of lot more of how not to use than just GoTo.

Aflatoon
03-19-2014, 09:42 AM
I know (it would probably make a good teaching tool) but it seemed apropos.

Bob Phillips
03-19-2014, 10:44 AM
How odd, I was going to use apropos in my previous response, but decided that such words were not within the bailiwick of VBAX :).

Paul_Hossler
03-20-2014, 06:10 AM
...not within the bailiwick of VBAX.

We'll give you bailiwick and call it a tie :devil2:


Paul

Aflatoon
03-20-2014, 07:06 AM
Perhaps the moderators should be retitled bailiffs? ;)