I have a few small modules where I have used If Then Goto statements (mostly for filters) as, at least to me, it is easier to read in a logical sense as you test for various conditions. However I have had several people tell me that using GoTo is a big no no but no one has told me why as of yet. I could see it being bad in large code blocks as it could make following what is happening when reading it very difficult.
Anyone have insights as to why I am being told this is bad practice and should avoid doing it?
Ah yes I am familiar with that term (even though most of the time I hear it used incorrectly).
So my seemingly simple (and what I thought easy to read) is considered bad practice. I will have to correct that then.
Here is what I was doing:
Code:
Private Sub PriceFilters()
Dim strfilter As String
If Me.MfrFilterTxt > "" And Me.PartTxtBx > "" Then GoTo Line1
If Me.MfrFilterTxt > "" Then GoTo Line2
If Me.PartTxtBx > "" Then GoTo Line3
If Me.MfrFilterTxt = "" And Me.PartTxtBx = "" Then GoTo Lastline
Line1:
strfilter = "[Manufacturer] like '*" & Me.MfrFilterTxt & "*'"
strfilter = strfilter & " AND [Part] like '*" & Me.PartTxtBx & "*'"
GoTo Lastline
My mind read that very easily, so I did not think it was bad but can see why it would be considered such.
Then I suppose the next question would be is there an even more "correct" method then:
Code:
If Me.MfrFilterTxt > "" Then
strfilter = "[Manufacturer] like '*" & Me.MfrFilterTxt & "*'"
Me.PriceHistorySubFrm.Form.Filter = strfilter
Me.PriceHistorySubFrm.Form.FilterOn = True
Me.PriceHistorySubFrm.Form.OrderBy = "[DateEntered] Desc"
Exit Sub
Else
strfilter = ""
Me.PriceHistorySubFrm.Form.Filter = ""
Me.PriceHistorySubFrm.Form.FilterOn = False
Exit Sub
End If
I just more or less repeat this and change the IF statement to look at what the controls contain. Should I go on step further and return strfilter as a variable into another subroutine?
Private Sub PriceFilters()
Dim strfilter As String
strfilter = ""
Me.PriceHistorySubFrm.Form.Filter = ""
Me.PriceHistorySubFrm.Form.FilterOn = False
If Me.MfrFilterTxt > "" And Me.PartTxtBx > "" Then
strfilter = "[Manufacturer] like '*" & Me.MfrFilterTxt & "*'"
strfilter = strfilter & " AND [Part] like '*" & Me.PartTxtBx & "*'"
ElseIf Me.MfrFilterTxt > "" Then
strfilter = "[Manufacturer] like '*" & Me.MfrFilterTxt & "*' "
ElseIf Me.PartTxtBx > "" Then
strfilter = "[Part] like '*" & Me.PartTxtBx & "*'"
End If
If Not IsEmpty(strfilter) Then
Me.PriceHistorySubFrm.Form.Filter = strfilter
Me.PriceHistorySubFrm.Form.FilterOn = True
Me.PriceHistorySubFrm.Form.OrderBy = "[DateEntered] Desc"
End If
End Sub
That looks so much better than the giant IF blocks I was doing.
This could end up being a good thread so I'm interested as well what others have to say about this. My own take is that using the goto's somewhere else unnecessarily, causes you to to scroll somewhere else on the code page to find out what that condition is actually doing. So you might know what it does, but someone else looking at the code doesn't and is force to maneuver around. In a regular subroutine that flows straight down the block of code logically, this is not the case. Look up "Collocation" in context of programming to learn more. It becomes even more interesting when mixed with object oriented code.
I see you posted an updated version which is more in line with standard procedural coding.
I usually design such a code a little differently - 4 lines per search field, and any combination of fields, for numeric fields and date fields of the beginning / end of the period, there are small changes in the code (there are no quotes or there are #)
moreover, only one boundary of the interval can be given or the condition OR is applied
Code:
Private Sub PriceFilters()
Dim s1 As String, s2 As String
''''''''''''''''''''''''''''
s2 = Trim("" & Me.MfrFilterTxt)
If Len(s2) > 0 Then
s1 = s1 & " and [Manufacturer] like '*" & s2 & "*'"
End If
''''''''''''''''''''''''''''
s2 = Trim("" & Me.PartTxtBx)
If Len(s2) > 0 Then
s1 = s1 & " and [Part] like '*" & s2 & "*'"
End If
''''''''''''''''''''''''''''
If Len(s1) > 5 Then
Me.PriceHistorySubFrm.Form.Filter = Mid(s1, 5)
Me.PriceHistorySubFrm.Form.FilterOn = True
Me.PriceHistorySubFrm.Form.OrderBy = "[DateEntered] Desc"
End If
''''''''''''''''''''''''''''
End Sub
Private Sub PriceFilters()
Dim strFilter As String
If IsNull(Me.MfrFilterTxt) = False Then
strFilter = strFilter & " AND [Manufacturer] like '*" & Me.MfrFilterTxt & "*'"
End If
If IsNull(Me.PartTxtBx) = False Then
strFilter = strFilter & " AND [Part] like '*" & Me.PartTxtBx & "*'"
End If
If Len(strFilter) > 6 Then
' First (in begining of string) " AND " - out!
strFilter = Mid(strFilter, 6)
Me.PriceHistorySubFrm.Form.Filter = strFilter
Me.PriceHistorySubFrm.Form.FilterOn = True
Else
Me.PriceHistorySubFrm.Form.Filter = ""
Me.PriceHistorySubFrm.Form.FilterOn = False
End If
End Sub
I have a few small modules where I have used If Then Goto statements (mostly for filters) as, at least to me, it is easier to read in a logical sense as you test for various conditions. However I have had several people tell me that using GoTo is a big no no but no one has told me why as of yet. I could see it being bad in large code blocks as it could make following what is happening when reading it very difficult.
Anyone have insights as to why I am being told this is bad practice and should avoid doing it?
many years ago, when I was a young specialist, I was given a task to understand the program
, it was spaghetti 20+ pages of 72 lines, which means more than 1500 lines
- absolutely uninformative variable names like a1,a2,.....k1,k2
- in addition to the usual labels, there were a lot of re-assigned labels (there are none in ACCESS)
it was something, I didn't understand
anything, after a few days I decided to draw a flowchart on a 82cm* 60cm watman, it's about 3 by 2 feet, 8 ordinary sheets of paper
after making half of the scheme, I realized that the tree has about 30 conditions of 20 lines with access to the formation of an reports with the addition of headers and signatures
yes, I rewrote it by introducing procedures and functions, the program decreased by 3 times, but it became absolutely transparent for understanding and reading
GOTO also remained in it to exit to the end of processing from the next condition
it was only after 2-3 years that I got acquainted with a book by a German author about structural programming, while I did not find anything new for myself
I wrote COBOL for 30 years with NO GoTo's. That was structured programming. GoTo's are not necessary. I was a convert then and haven't changed my opinion yet and so still do not use them except for error handling. Like VBA, the problem with CICS was that it handled errors badly so you could not avoid using GoTo's. At a minimum, you had to GoTo the error handler.
Keep in mind that COBOL is not event driven so you had a mainline that was an outline of the process and essentially a list of Perform's (GoSub's). The exit from the program was at the end of the mainline.
I use GoSub's in VBA to organize long procedures or for code that will be reused within the sub or function. Code that is reused by other parts of an application need to be subs or functions defined in standard modules.
I wrote COBOL for 30 years with NO GoTo's. That was structured programming. GoTo's are not necessary. I was a convert then and haven't changed my opinion yet and so still do not use them except for error handling. Like VBA, the problem with CICS was that it handled errors badly so you could not avoid using GoTo's. At a minimum, you had to GoTo the error handler.
Keep in mind that COBOL is not event driven so you had a mainline that was an outline of the process and essentially a list of Perform's (GoSub's). The exit from the program was at the end of the mainline.
I use GoSub's in VBA to organize long procedures or for code that will be reused within the sub or function. Code that is reused by other parts of an application need to be subs or functions defined in standard modules.
Coming as I did from an Assembly Language environment (device drivers for DEC PDP-11) and some other aseembly environments, I never used a GOTO because it was a JUMP (or JMP or BRANCH or pick another word - and they usually did). But TRUST me, they WERE necessary at that time.
My main complaint regarding GOTO is that there are times for exit/abort conditions where you have incredibly complex nesting of IF ladders or SELECT CASE (or COMPUTED GOTO from FORTRAN) where a GOTO helps to deconvolute the spaghetti. (How did Alexander untie the Gordian Knot? He cut through it ignoring its loops!) Modern languages so do much better than the early 3rd-generation languages with loop control and (what used to be called) alternation control, and thus have drastically reduced the number of situations where that radical level of deconvolution is so beneficial - and necessary.
I would also suggest that for some of those earlier "high-level" languages, GOTO was the "get out of jail free" card. It took a while for some of those procedural languages to add the escape hatches like EXIT FOR or EXIT DO etc. As those were added, the need for GOTO diminished. But we got here by a gradual improvement, not an overnight switch from REALLY dumb languages to smarter languages. I would suggest that if you don't like GOTO but DO use EXIT FOR, EXIT DO, early EXIT SUB, etc. then you are playing at semantics. You have replaced one verb for another.
Therefore, I am a believer in GOTO only when too many options present themselves and the convolution factor even WITHOUT the GOTO becomes too complex to follow. Before anyone says "just design it better" I ask you to remember that the situation drives the program; the programmer doesn't always drive the situation. But I agree that the number of GOTOs required to do things is reducing dramatically.
@tmyers
It is very common in Access VBA to do something like you are doing and build concatenated strings for filters, sql, rowsources, etc. There are lots of techniques but the trick is to handle one or more conditions that does not have a value. You have to do one thing if it has a value or another if it does not. You can do this trick where you remove an And or OR from the beginning or remove something from the end. I find it just a lot easier to use this construct.
Dim str as string
..
Code:
if str = "" then
Do something
else
str = str & do something
end if
As you loop or check controls, you simply check if the concatenated string has been populated.
I added a third case to be a little more descriptive
Code:
Private Sub PriceFilters()
Dim strFilter As String
Dim Mfr as string
dim Part as string
dim frm as access.form
Mfr = Me.mfrFiltertxt & ""
part = me. PartTxtBx & ""
If mfr = <> "" Then
strFilter = strFilter & "[Manufacturer] like '*" & Mfr & "*'"
End If
If Part <> "" Then
if strFilter = "" then
strFilter = strFilter & "[Part] like '*" & Part & "*'"
else
strFilter = strFilter & " AND [Part] like '*" & Part & "*'"
end if
End If
If SomethingElse <> "" then
if strFilter = "" then
strFilter = strFilter & "[Something] like '*" & Something & "*'"
else
strFilter = strFilter & " AND [Something] like '*" & Something & "*'"
end if
end if
set frm = me.priceHistorysubfrm.form
frm.Filter = strFilter
frm.FilterOn = (strFilter <> "")
End Sub
A couple minor issues that can make writing code easier. Short is not necessarily better, but it can create less chance of error.
1. If you reference a long object more than once, consider a variable instead of repeating
add a variable for pricehistorysubform.form
Code:
dim frm as access.form
set frm = me.pricehistorysubform.form
then just use frm throughout
2. If the result of an "if then" causes a switch from a true condition to a false condition you can make a "toggle" with one line of code
Code:
If Len(strFilter) > 6 Then
Me.PriceHistorySubFrm.Form.FilterOn = True
Else
Me.PriceHistorySubFrm.Form.FilterOn = False
End If
to simply
Code:
frm.filteron = len(strFilter) > 6
3. You do not have to set the default values of variables
Code:
Dim strfilter As String
strfilter = ""
That second line makes no sense. A string defaults to "", numbers to 0, objects to null, booleans to false.
4. Empty controls are usually NULL not "". They can be "", but that is rare and you have to force that to happen.
This is not a good check since rarely = ""
Code:
Me.MfrFilterText = ""
Better is:
Code:
If isNull(me.mfrfiltertxt)
Even better is:
Code:
If (Me.mfrfiltertext & "") = "".
This checks both empty cases (null and "")
The best checks is probably
Code:
Trim(Me.mfrTextFilter & "") = ""
That checks a third case of one or more spaces.
Never do If
Kind Galina: "many-many years ago ... " - when? - first PC is just from 1983 (if I remember correctly)
You are too young to say so, many happy years to you!
...
Have you ever calculated the shortest distance (more then 300 miles on the surface of the Geoid) on the Great Circle Arc by a logarithmic ruler?
