Why is using GoTo considered bad practice?

[MarkK]

Another idea about how to construct and apply a filter...

Private Function GetFilterClause(ctrl As Control, FieldName As String) As String
    If Nz(ctrl.Value, "") <> "" Then GetFilterClause = "And [" & FieldName & "] LIKE '*" & ctrl.Value & "*'"
End Function


Private Property Get MfrClause() As String
    MfrClause = GetFilterClause(Me.tbManufacturer, "Manufacturer")
End Property

Private Property Get PartClause() As String
    PartClause = GetFilterClause(Me.tbPart, "Part")
End Property

Private Property Get OtherClause() As String
    OtherClause = GetFilterClause(Me.tbOther, "Other")
End Property


Private Property Get FinalFilter() As String
    Dim tmp As String
    tmp = MfrClause & PartClause & OtherClause
    If Len(tmp) Then FinalFilter = Mid(tmp, 5)
End Property


Sub SetFilter(State As Boolean)
    If State Then Me.Filter = FinalFilter
    Me.FilterOn = State
End Sub

A clause assembly function, a scalable series of filter clause properties, and final assembly.

 

[Eugene-LS]

Another idea about how to construct and apply a filter...

Good idea!
But the filter can be built with different types of form objects (textBox, comboBox, listBox, listBox with multi selection) ...
They are handled differently ...

 

[Auntiejack56]

I avoid gotos because they don't correspond to a way of seeing a logical solution to a problem. I'd hope that other folk would read my code and follow the train of thought from top to bottom in each routine. Not jump around like a jack-in-a-box.
I must admit, though, that validating screen data sometimes defeats me, given how users see the interrelationships and dependencies between data elements on a single form.

 

[Mike Krailo]

GoTo is not the same for every language or implementation (depends on reason for using it). In C# the GoTo doesn't just go anywhere, it breaks out of inner loops so it can be viewed there as a good thing. If the GoTo's use only goes a few lines down the code page and it is easily readable and has a good reason to be used, then fine, use it. Here is an interesting article from a Benjamin Foreback (Why should GoTo be avoided?)that works with Geo Coordinates in the defense industry. He advises to avoid using them but acknowledges that certain circumstances will make exceptions to the rule. This is not VBA specific but GoTo is in many different languages so it's interesting to get different perspectives on it.

 

[The_Doc_Man]

Thanks for the link, @Mike Krailo - the author is like me. Sometimes you do it to break out of a bad situation. It is the pragmatic solution to any of several problems caused by convoluted nesting. When I think back about it, I realize that most of my GOTOs were "escape from trouble" situations, but with one exception.

In the world of Digital Equipment Corporation, there was Digital Control Language (DCL), a scripting language that was incredibly powerful but syntactically sparse in some odd cases. Because they were scripted and thus interpreted, sometimes things got a little dicey and the GOTO was needed just to keep from getting totally stuck. I cannot BEGIN to tell you how many times I had batch scripts that managed my machines for me. Including my VAXen and their successors, the Alpha and the Itanium processors. They were incredible systems and with a little bit of ingenuity I managed to keep tabs on their operations through batch oriented monitoring routines. There, a GOTO was the only solution because they didn't have good looping abilities so I had to "roll my own." In that case, GOTO was mandated by the environment, which had all the basics but none of the bells and whistles. That, they left to the code authors.

 

[KitaYama]

Unlike most of you, I don't care jumping up and down. I like to create an image of what the whole function is supposed to do. So my code mostly seems like a map. If anyone sees the code, he definitely will understand what I'm trying to do and how.
I like to keep my code clean and tidy. So a train of code from top to down doesn't work for me. I'm not afraid of jumping around like a jack-in-a-box because the editor can be split. The top half of the editor is set to show the map, and the bottom half I can move through different sections of the cod.

Every morning I have to send a mail with an embedded table about previous manufacturing result. The following is a part of the code used for this purpose. I know you pros like you don't like this method, but I absolutely like it.

    Dim Msg As String
    Dim Receiver As String
    Dim Attachment As String
    Dim Subject As String
    Dim CC As String
    Dim AddAttachment As Boolean
    Dim Atta As String
    Dim fltr As String
    Dim rs As DAO.Recordset
   
    If TestMode Then
        Stop
    Else
        On Error GoTo ErrorTrap
    End If
       
    Msg = ""
    GoSub SetFilter
    GoSub AddHtmlHeader
    GoSub AddMailHeader
    GoSub AddTableHeader
    GoSub AddTableRows
    GoSub AddTableEnd
    GoSub GetSignature
    GoSub AddAttachment
    SendMail_Attach True, Receiver, Attachment, Subject, Msg, CC, True, False
     
    Exit sub

Set Filter:
    ......
    ......
    ......
    Return
   
AddMailHeader:
    Msg = Msg & "<p>暲栘恀旤 條<br/>"
    Msg = Msg & "<p>"
    Msg = Msg & DLookup("CompanyName", "tblOrderedFrom", "OrderedFrom_PK=" & OrderedFrom_FK)
    Msg = Msg & "  偐傜怴偟偄拲暥傪捀偒傑偟偨偺偱拲暥彂傪揧晅偟偰傑偡丅<br/>"
    Msg = Msg & "偍朲偟偄強怽偟栿偁傝傑偣傫偑丄彜嵃偺搊榐傪媂偟偔偍婅偄抳偟傑偡丅<br/><br/></p>"
    Return
   

AddTableHeader:
    Msg = Msg & "<table>"
    Msg = Msg & "<tr>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='130'>庴拲斣崋</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='150'>拲斣</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='150'>惢斣</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='150'>恾斣</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='60'>悢検</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='80' style='text-align:center'>扨壙</th>"
    Msg = Msg & "<th bgcolor = #E0F8F1 WIDTH='120'>婓朷擺婜</th>"
    Msg = Msg & "</tr>"
    Return


AddTableRows:
    ' Create the table rows
    Set rs = CreateRs_DAO("qryOrders_Products", fltr)
    With rs
        If .EOF Then Return
        Do
            Msg = Msg & "<tr>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='130'>" & !ID & "</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='150'>" & !OrderNo & "</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='150'>" & !ManufacturingNumber & "</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='150'>" & !DrNo & !OrderDrNoChanges & "</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='60'>" & !Quantity & "屄" & "</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='80' style='text-align:right;padding-right:15px'>\" & Format(!UnitPrice, "#,###") &"</td>"
            Msg = Msg & "<td bgcolor = #F5F6CE WIDTH='120'>" & !Delivery & "</td>"
            Msg = Msg & "</tr>"
            .MoveNext
        Loop Until .EOF
    End With
    Return

Anyone who sees the code, can imagine how it works. If I need to change some section of it, I can jump right to the appropriate code.
As I said, the first GoSub Section works like a map for the code. It tells me what I'm trying to do in each step. If I need to change the sequence, I simply can drag a GoSub to a higher or lower place.

 

[Mike Krailo]

@KitaYama Although GoSub is similar to GoTo, they are definitely not the same. GoTo goes and doesn't come back, while each labeled section of code using GoSub returns back to whence it came. That's fine with me and has a certain easy on the eye look to it as you see what each block of code is doing overall at the top. Most functions are not huge so you don't see the need for that type of thing, but if it is a large function with many parts, it usually could be broken down into smaller subroutines or functions. Now if the functions are not reusable in anyway, then what you are doing is perfectly fine. If you can create reusable functions that will work on their own in virtually any application, those should definitely be pulled out.

Here is an interesting stackoverflow discussion on refactoring in general that talks about size of functions.

 

[Pat Hartman]

The size of a function or sub is as large as it needs to be to perform ONE action. Attempting to make it smaller by dividing it up is very similar to making a bunch of 1-1 relationships because you have too many columns for one table. If you properly defined the function, it should NOT be broken up. A client once asked me if my team could produce the finished product more quickly if instead of nine separate pieces of functionality, we combined a couple of them. I wasn't snarky when I answered but the thought that crossed my mind was maybe if we just added more "man" power, nine women could make that same baby in one month

It is when you don't understand the concepts of coupling and cohesion that you end up with huge procedures and the need for branching. When you actually accept the discipline of not using GoTo's, your code style changes. You decide BEFORE you enter a routine whether you want to perform it or not rather than the waterfall of straight line code where you decide after you're in the procedure that you shouldn't be doing this and need to branch out. My code frequently looks like what @KitaYama posted. That style with good procedure names, makes it very easy to find the code you are interested in and understand its full scope. It's like a bunch of black boxes within a larger black box. THAT is how you organize a large procedure to make it understandable.

 

[Isaac]

Honestly (I'm being sincere, not sarcastic) the easiest way to become convinced to agree with avoiding using it is probably to do this:

Begin using it liberally in code that you're still working on developing. Something that won't be super simple but will evolve a bit and become at least moderately complex. After a while you'll become a believer.

It's really not that there is anything "bad" about a single GoTo. The problem is that GoTo's as a logical construct tend to immediately begin spawning. 1 will turn into 3, and all of a sudden you'll realize that the 3 must become 15 in order for everything to work. Trust me, it becomes a nightmare slippery slope.

I use them once in a great while, but normally there are better ways to design your control-of-flow.

Then again, improving the quality of your control-of-flow is an infinite life's work, as we are always getting better and improving.

For example I personally DO use Exit Function pretty liberally in my functions, while that is recommended to avoid (in favor of taking more time to think it through but I get lazy sometimes).

 

[Isaac]

What Pat said about [what I would call Modular-ness] is also good. And creating portable single-use functions is generally a good thing that will eliminate a lot of different bad habits and bad results (not just GoTo's).

Everything in moderation. I will say there have been 2 times in the past decade when I've had to troubleshoot other people's code and they took that single-use idea to a bit of an extreme (since there is no black and white way to define it - since the "atom" can almost always be split into smaller pieces) - except with the code I am thinking of, it couldn't. I mean these people split things into so many functions there were hundreds of single-line-of-code functions. They wouldn't even type If left(accountnumber,2)='TX' without having a function called GetState() (or something).
Worse yet, these people really liked short, cryptic names. Many functions were called X, Y, x2, etc. I'm a firm believer in descriptive nomenclature - that a variable or function name is NOT a "golden opportunity" to save yourself a few letters of typing (if you don't type fast you're in the wrong business to begin with)---Rather, variable names are a chance to be DESCRIPTIVE, so that your code can be read and understood with minimal digging. It's all fun and games until you're 2 pages down from the Declarations area and can't remember what Y vs. G means.

But definitely go in that direction, your intuition will probably tell you when to stop. Part of that intuition will arise from the pain of troubleshooting. When you are troubleshooting a problem and have to step 'out' 20 levels deep, you may wish to do some collapsing. But that's another story

 

[sonic8]

I like to create an image of what the whole function is supposed to do. So my code mostly seems like a map. If anyone sees the code, he definitely will understand what I'm trying to do and how.

Your basic idea is not bad. However, why don't use procedures instead of the jump labels? They would allow you to define clear interfaces to pass the data around, instead of keeping all data mashed together in the variables of your much too large procedure?

 

[Mike Krailo]

Your basic idea is not bad. However, why don't use procedures instead of the jump labels? They would allow you to define clear interfaces to pass the data around, instead of keeping all data mashed together in the variables of your much too large procedure?

The only thing I could think of why he did it that way is the common set of variables listed at the top which end up being used at the very end of it all.

 

[KitaYama]

why don't use procedures instead of the jump labels?

The answer is simple. Because I don't want to make a mess out of my code. @Pat Hartman summed it up.
And exactly as what @Mike Krailo mentioned.
I don't add additional procedures or functions when I don't re-use them. That's the whole point.

Let me give you an example.
I have 10 GoSubs and they use a pool of variables. The variables are available in each section of my code and I can use/change them whenever I need them. Now if I want to change them to 10 subs or functions, I have to pass the variables to the functions or re-create them by recordsets, DoLookups, etc. I also have to be careful the data passed to functions are Byval or ByRef.

Further more I have to add additional variables in new subs to be able to work on them. Then I have the problem of returning values. As long as the function returns one set of data, that's OK. But what if the functions need to return two sets of data. I have to use a delimiter and concatenate the result. Then I have to split the returned value in my main sub to extract each part. And then there's the problem with returned data type. Because concatenated data is string.
And you see? I'm adding to my headache by complicating the situation.

Several members explained they don't like jumping up and down. Shortening a long function to sub functions prevents them jumping up and down. But now they have to jump here & there between functions. And there's only one difference. Instead of simply scrolling the editor, now they have use the search function to move between procedures or use the drop down above page.

Again : If I feel I can reuse a section of my code, it definitely goes into a new function. But just tearing apart a long procedure to smaller procedures just for the sake of shortening it, is heating up the maintenance of the code. (at least for me)
I really don't see any benefits for using procedures.

 

[sonic8]

The size of a function or sub is as large as it needs to be to perform ONE action.
[...]
My code frequently looks like what @KitaYama posted. That style with good procedure names, makes it very easy to find the code you are interested in and understand its full scope. It's like a bunch of black boxes within a larger black box. THAT is how you organize a large procedure to make it understandable.

Your post appears to contradict itself. If you can segment a procedure into 8(!) different sections using jump labels, as @KitaYama is doing, it is very clearly performing much more than just one action.

 

[sonic8]

I have 10 GoSubs and they use a pool of variables. The variables are available in each section of my code and I can use/change them whenever I need them. Now if I want to change them to 10 subs or functions, I have to pass the variables to the functions or re-create them by recordsets, DoLookups, etc. I also have to be careful the data passed to functions are Byval or ByRef.

All this is a strong argument for using individual procedures to me.

Further more I have to add additional variables in new subs to be able to work on them. Then I have the problem of returning values. As long as the function returns one set of data, that's OK. But what if the functions need to return two sets of data. I have to use a delimiter and concatenate the result.

1. In the code shown above, it appears there would be only a single return value for each procedure made out of one of your jump labels.
2. You can output multiple distinct values by using ByRef arguments or, usually better, return a complex type from a function.

Several members explained they don't like jumping up and down. Shortening a long function to sub functions prevents them jumping up and down. But now they have to jump here & there between functions. And there's only one difference. Instead of simply scrolling the editor, now they have use the search function to move between procedures or use the drop down above page.

A well named procedure with a clear set of arguments and/or a return value should drastically reduce the need to jump to the definition of the function.
Your "pool of variables" would be the primary reason for jumping around your code for me. This would be eliminated with individual procedures. You could then treat each variable as required by the individual procedure without having to consider the rest of the code using that pool as long as the signature/interface to the procedure does not change.

 

[KitaYama]

@sonic8 For sure you're much better than me in this. So instead of answering your points, I prefer to ask you a question.
Can you explain the benefits of using individual functions when you don't plan to reuse them? (instead of my jump labels)

Thanks.

 

[SHANEMAC51]

Can you explain the benefits of using individual functions when you don't plan to reuse them? (instead of my jump labels)

I can't even remember when and in what program I used goSub, apparently in fortran in 1980, I definitely didn't use it in ms access

I think goSub unloads the main part of the code from unnecessary details, but for other parameter values, you will have to write a new goSub

the function does the same thing, but it can accept the parameters passed when it is called

AddMailHeader:
    Msg = Msg & "<p>title1<br/>"
    Msg = Msg & "<p>"
    Msg = Msg & DLookup("CompanyName", "tblOrderedFrom", "OrderedFrom_PK=" & OrderedFrom_FK)
    Msg = Msg & "  title2<br/>"
    Msg = Msg & "title3<br/><br/></p>"
    Return


function AddMailHeader(title1,title2,title3) as string
    Msg = Msg & "<p>" & title1 & "<br/>"
    Msg = Msg & "<p>"
    Msg = Msg & DLookup("CompanyName", "tblOrderedFrom", "OrderedFrom_PK=" & OrderedFrom_FK)
    Msg = Msg & "  " & title2 & " <br/>"
    Msg = Msg & title3 & "<br/><br/></p>"
    Return

 

[KitaYama]

I think goSub unloads the main part of the code from unnecessary details, but for other parameter values, you will have to write a new goSub

I really don't understand what you mean by the above bold section.
thank you.

 

[MajP]

Can you explain the benefits of using individual functions when you don't plan to reuse them? (instead of my jump labels)

Debugging and readability! You should be able to write and test each part separately.

 

[KitaYama]

Debugging and readability! You should be able to write and test each part separately.

I can understand the testing.
What do you mean by readability? Aren't each sections of GoSubs readable to you?