How should I rewrite this to not use GoTo?

[planbattack]

How should I rewrite the following code without using GoTo?
My actual array "arr" is much more complicated than this; I have simplified it for illustration.
Basically, I want to be able to define a list of "skip" strings in a way that is concise and easy to add to or subtract from later on if needed (like I have here).
Then, when I cycle through my main arr, I want to skip to the next element if it matches any one of my skip strings.
In my example, the "do stuff" code is only executed for elements "US" and "UK".

My current code works fine in VBA, but I know GoTo should be avoided where possible because it should never be necessary.
What is the proper way to write such a code?

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then GoTo oNexti
  Next
   'do stuff
oNexti:
Next

Thanks in advance.

[Elroy]

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then j = UBound(arrSkip)
  Next
   'do stuff
oNexti:
Next

That's one way to make your code work. Another is just to execute an Exit For instead of change j. Another that comes to mind is a "dummy" Do/Loop that you allow to execute only once. Another way is to have a boolean flag variable that you check to see if an Exit For is called for just before the loop executes again.

EDITED: Here, I'll provide untested examples of all of those:

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then Exit For
  Next
   'do stuff
oNexti:
Next

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  Dim b as Byte: b = False
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then b = True
  Next
   'do stuff
  If b then Exit For
oNexti:
Next

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  Do
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then Exit Do
  Next
   'do stuff
oNexti:
  Exit do
  Loop
Next

[Niya]

How should I rewrite the following code without using GoTo?
My actual array "arr" is much more complicated than this; I have simplified it for illustration.
Basically, I want to be able to define a list of "skip" strings in a way that is concise and easy to add to or subtract from later on if needed (like I have here).
Then, when I cycle through my main arr, I want to skip to the next element if it matches any one of my skip strings.
In my example, the "do stuff" code is only executed for elements "US" and "UK".

My current code works fine in VBA, but I know GoTo should be avoided where possible because it should never be necessary.
What is the proper way to write such a code?

arr = Split("US|UK|JP|CN", "|")
arrSkip = Split("JP|CN|KR", "|")

For i = 0 To UBound(arr) 
  For j = 0 To UBound(arrSkip)
    If Split(arr(i) = arrSkip(j) Then GoTo oNexti
  Next
   'do stuff
oNexti:
Next

Thanks in advance.

Yea, this kind of thing is really bad in my opinion.

Firstly the performance of search structured that way is horrible. For a small number of items, it won't matter much but it might be a good idea to get into the habit doing it right. This looks like a perfect scenario for a HashSet object:-

        Dim arr = "US|UK|JP|CN".Split("|")
        Dim skip As New HashSet(Of String)("JP|CN|KR".Split("|"))

        For Each item In arr
            If Not skip.Contains(item) Then
                'This item is not to be skipped
            End If
        Next

The above is VB.Net code but the same principle can be applied in VBA or VB6. A HashSet is designed to perform extremely fast look-ups. Perhaps someone has a good implementation of such an object for VB6 and VBA users. There might be one in the CodeBank here. You could also use a Collection or a Scripting.Dictionary as a substitute as these objects also perform very fast look-ups. The only difference is that these objects require each item to have a key while a HashSet does not.

As for your more general problem of structure. You can get rid of the GoTo by pawning off the inner search to a function. Here a sample of what it could look like:-

Option Explicit
Private Sub Form_Load()
    Dim arr() As String
    Dim arrSkip() As String
    Dim i As Long

    arr = Split("US|UK|JP|CN", "|")
    arrSkip = Split("JP|CN|KR", "|")
    
    For i = 0 To UBound(arr)
        If Not ArrayContains(arrSkip, arr(i)) Then
            Debug.Print arr(i)
        End If
    Next

End Sub

Private Function ArrayContains(arr() As String, ByVal item As String) As Boolean
    Dim i As Long
    For i = LBound(arr) To UBound(arr)
        If arr(i) = item Then
            ArrayContains = True
            Exit For
        End If
    Next
End Function

The above code outputs this:-

US
UK

[argen]

arr = Split("US|UK|JP|CN", "|")
sSkip = "|JP|CN|KR|"

For i = 0 To UBound(arr)
    If InStr(sSkip, "|" & arr(i) & "|") = 0 Then
        'do stuff
    End If
Next

[jpbro]

Here's a couple of other ways you could do it:

Sub Test()
   Dim skip As String
   Dim arr() As String
   Dim arrSkip() As String
   Dim i As Long
   Dim j As Long
   
   skip = "JP|CN|KR"
   
   arr = Split("US|UK|JP|CN", "|")
   arrSkip = Split(skip, "|")
   
   For i = 0 To UBound(arr)
     For j = 0 To UBound(arrSkip)
       If arr(i) = arrSkip(j) Then Exit For   ' Short-circuit on match
     Next
     
     If j > UBound(arrSkip) Then
       ' The j loop went through every combo and didn't find a match (so j is now > ubound of the skip array)
       ' This means we ca Do Stuff
       ' If a match had been found, our short-circuit will have left j <= ubound of the skip array, so we won't Do Stuff
     End If
   Next
End Sub

OR

Sub Test()
   Dim skip As String
   Dim arr() As String
   Dim i As Long
   
   skip = "JP|CN|KR"
   
   arr = Split("US|UK|JP|CN", "|")
   
   If Right$(skip, 1) <> "|" Then skip = skip & "|"   ' Not strictly necessary if you will also have 2 character country codes
   For i = 0 To UBound(arr)
      If InStr(1, skip, arr(i) & "|") < 1 Then
         ' Do Stuff
      End If
   Next
End Sub

[Schmidt]

For "small stuffs" like that, the built-in Filter-Function could be applied as well:

Option Explicit

Private Sub Form_Load()
  Dim Arr: Arr = Split("US|UK|JP|CN", "|") 'init the Src-Arr
  
  RemoveFrom Arr, "JP|CN|KR" 'apply the Filter
  
  Debug.Print Join(Arr, vbLf) 'check the now filtered Arr
End Sub

Private Sub RemoveFrom(Arr, ByVal sFlt, Optional sDelim = "|")
  For Each sFlt In Split(sFlt, sDelim)
      Arr = Filter(Arr, sFlt, False, vbTextCompare)
  Next
End Sub

HTH

Olaf

[The trick]

It is't bad practice to use goto to emulate continue statement.

[Niya]

It is't bad practice to use goto to emulate continue statement.

I don't think it's bad per-se but it is ugly as hell. GoTo makes it a bit more difficult to follow what a piece of code might be doing. I'm always in favor of removing them where ever possible. Others might feel differently though. GoTo is one of those things in the domain of programming that people tend to get religious about.

[baka]

the importance when u use Goto or Exit Sub/Function is to remember to "close" any "with"
example:

With MyUDT
If .Item1 = 0 and .Item2 = 5 Then Goto toEnd
- code -
toEnd:
End With

so u don't exit the With without "ending it".
this is true for anything u do inside the function that need termination.

I think Goto is quite nice to have, but moderately used. it can help a lot when theres a lot of code u need to skip and u can't just exit or u don't want to create multiple functions.
remember that we use that with "On Error", that is also of course only used when we can't escape for such occurrence. so we should not "avoid it like its bad", but only use it when its the right thing to do.

[Elroy]

Personally, I think there's one (and ONLY one) place where GoTo is appropriate, and the above OP's code is not one of them.

The one place it's appropriate is where we do a lot of "setup" in a procedure (maybe get RecordSets open, or initialize GDI+, or whatever), that must subsequently be "cleaned-up". In those procedures, there might be several instances where you're done, and it's time to get out (with the clean-up). Without possible severe nesting (and indentation), it's often not practical to find ways out other than a GoTo (Jump to where the clean-up is done and then exit).

That's the only condition where I think it's appropriate.

However, as was stated, there are differing opinions.

[Niya]

The one place it's appropriate is where we do a lot of "setup" in a procedure (maybe get RecordSets open, or initialize GDI+, or whatever), that must subsequently be "cleaned-up".

Even in this scenario GoTo is unnecessary in 2022. We use Try/Finally blocks for this need. Every major language has this now. Here is an example in Python:-

# Online Python compiler (interpreter) to run Python online.
# Write Python 3 code in this online editor and run it.
def mySub():
    
    try:
    
        print("Doing something that needs to be cleaned up")
        
        #Notice we return from this sub before
        #without needing to explicitly call
        #out clean up code. Finally will make sure it always
        #runs leaving the try block
        return
    finally:
        print("Cleaning up")


print("Entering Sub")
mySub()
print("Exiting Sub")

You can test it here. This is the output:-

Entering Sub
Doing something that needs to be cleaned up
Cleaning up
Exiting Sub

Here is the same thing in JavaScript:-

// Online Javascript Editor for free
// Write, Edit and Run your Javascript code using JS Online Compiler
    
function MySub()
{
    try
    {
        console.log("Doing something that needs to be cleaned up");   
        return;
        
    }
    finally
    {
        console.log("Cleaning up");
    }
}

    
console.log("Entering sub");
MySub();
console.log("Exited sub");

You can also do it in C#, VB.Net, Java and PHP to name a few. Notable exceptions are C and C++ but even here Microsoft compilers have Microsoft specific extensions to support it.

Point is, GoTo is necessary to support clean-up in functions with multiple exits only in older syntaxes like VB6 and VBA. Most programmers won't have this problem because whatever language they're using will most likely have try/finally support.

[baka]

but we need to remember that goto is not bad. even if theres many other ways to do things and new methods, goto can still be used if the programmer likes it.
it will not create slow code, contrary, it could actually increase performance.
I say, do whatever u want, as long it works and is at least as fast as anything else.

[Niya]

Well I mean people can use them if they want. Objectively speaking, there is nothing wrong with them but personally I hate seeing them and avoid them at all costs. It's really just about aesthetics for me.

[dz32]

if the loop is really long I actually prefer to use a goto label sometimes.
usually when I dont want to nest if statements another layer deep and not when an exit for will suffice.

[dilettante]

    Dim arr() As String
    Dim arrSkip() As String
    Dim I As Long
    Dim J As Long

    arr = Split("US|UK|JP|CN", "|")
    arrSkip = Split("JP|CN|KR", "|")

    For I = 0 To UBound(arr)
        For J = 0 To UBound(arrSkip)
            If arr(I) = arrSkip(J) Then Exit For
        Next
        If J > UBound(arrSkip) Then
            'do stuff
            Debug.Print arr(I)
        End If
    Next

[baka]

this is one of my many functions using goto:

Function world_ExamineMap() As Boolean
    With Character(Selected).Location
        If link(.world).map(.map).Mode < 4 Then world_ExamineMap = True
        If .world = 0 Then GoTo toEnd
        If sys.safe Then Character(Selected).safe = Character(Selected).Location
        If Character(Selected).world(.world).map(.map) > 0 Then world_ExamineMap = True
        If .sub > 0 Then world_ExamineMap = True: GoTo toEnd
        If world_ExamineMap Then
            map(.map).open = True
            Character(Selected).world(.world).map(.map) = 1
            GoTo toEnd
        End If
        mob_CalculateTimer
toEnd:
    End With
End Function

I mean, try to make this without Goto.
and remember I can't use "Exit Function" because I need to close "with".

[Niya]

What happens if you don't close the With block?

[baka]

Character(Selected).Location will get locked.
I can still "read" but I can not do any memory changes, like redimming if the UDT has that.
if you are just reading, it will not cause errors, but its good practice to close the With block.

so, we could ask, "why" is the With block still open "after" we exit the function.
we could make is similar to "Open" method. that will lock the file until we use the "Close" method.

not sure "how long" the with block is locked. I have not tested.

[vbLewis]

Here is another way you could do it, this is just a variation of elroys method using a flag

    Dim Arr() As String
    Dim Skip() As String
    Dim M As Variant, S As Variant, F As Boolean


    Arr = Split("US|UK|JP|CN", "|")
    Skip = Split("JP|CN|KR", "|")

    For Each M In Arr
        F = False
        For Each S In Skip
            If M = S Then
                F = True
                Exit For
            End If
        Next
        If Not F Then
            MsgBox "do something with " & M
        End If
    Next

[techgnome]

this is one of my many functions using goto:

Function world_ExamineMap() As Boolean
    With Character(Selected).Location
        If link(.world).map(.map).Mode < 4 Then world_ExamineMap = True
        If .world = 0 Then GoTo toEnd
        If sys.safe Then Character(Selected).safe = Character(Selected).Location
        If Character(Selected).world(.world).map(.map) > 0 Then world_ExamineMap = True
        If .sub > 0 Then world_ExamineMap = True: GoTo toEnd
        If world_ExamineMap Then
            map(.map).open = True
            Character(Selected).world(.world).map(.map) = 1
            GoTo toEnd
        End If
        mob_CalculateTimer
toEnd:
    End With
End Function

I mean, try to make this without Goto.
and remember I can't use "Exit Function" because I need to close "with".

Function world_ExamineMap() As Boolean
    With Character(Selected).Location
        If link(.world).map(.map).Mode < 4 Then world_ExamineMap = True
        If .world = 0 Then GoTo toEnd
        If sys.safe Then Character(Selected).safe = Character(Selected).Location
        If Character(Selected).world(.world).map(.map) > 0 Then world_ExamineMap = True
        If .sub > 0 Then world_ExamineMap = True: GoTo toEnd
        If world_ExamineMap Then
            map(.map).open = True
            Character(Selected).world(.world).map(.map) = 1
            'GoTo toEnd
        Else
            mob_CalculateTimer
        End If
'toEnd:
    End With
End Function

Seemed simple enough to me.

-tg

[baka]

theres 2 more GoTo to remove. but sure I can use if/else, but the code will not seem nicer and easier to read.
and this was one of the smaller one, I have more complex functions

[Niya]

Character(Selected).Location will get locked.
I can still "read" but I can not do any memory changes, like redimming if the UDT has that.
if you are just reading, it will not cause errors, but its good practice to close the With block.

so, we could ask, "why" is the With block still open "after" we exit the function.
we could make is similar to "Open" method. that will lock the file until we use the "Close" method.

not sure "how long" the with block is locked. I have not tested.

Hmmm. That doesn't sound right. The only way I know of on Windows to prevent writing to memory is through VirtualProtect or VirtualAlloc. What you need to understand is that these functions operate on page granularity which is to say they only work in multiples of the Windows page size. A typical Windows page is 4096 bytes in size. The vast majority of objects and structures in any program at any given time will be way smaller than this. That means that if With blocks are locking memory, most of the time it would be locking memory that has nothing to do with the object or UDT in question. They would not do this because our programs would be crashing constantly because there is a very high chance that you're going to access a variable or object from inside the With block that sits on the locked page. Setting all this aside, still, it's bad form for a compiler to be locking pages like this implicitly. I can't imagine anyone would actually design a compiler to do this. There would be rioting in the streets!

Now this is not to say that you're wrong. Based on what I know about some of the low level details behind how programs are structured, how instruction set works etc. I am 99% certain that the compiler is not using VirtualProtect to lock stack or heap memory during the execution of a program. So what is happening? Well when I think about it, it could be COM's fault. The only COM entity I'm aware of that is lockable is the SAFEARRAY. It could be locked through the SafeArrayLock function. So With blocks could be calling SafeArrayLock if the type in question has an array. The only other thing I can think of is that COM objects have some kind of standard way to prevent access to objects. Perhaps through some standard interface but I've never heard of this. Perhaps the trick, Olaf or wqweto can tell us if such a thing exists in COM.

All in all, I strongly believe this locking you're talking about has to do with arrays being locked. In this case, I'd say it's safe to exit a With block unless an array is involved somehow. But all this is pending further testing. I actually find this interesting and I will probably test this myself later on to see what's really happening. Like I said, I seriously doubt memory is actually being locked. It's probably just COM being stupid.

[dilettante]

Blockexit clears any dangling temporary locked pointer from abuse of With. As far as I can recall forgetting how With works is the only real hazard. Better yet, avoid using UDTs as much as possible.

But this works just fine:

Option Explicit

Private Type Fudd
    Dudd As String
End Type

Private Fudd As Fudd

Private Sub Diddle()
    With Fudd
        .Dudd = "Diddle"
        Exit Sub
    End With
End Sub

Private Sub Doodle()
    With Fudd
        .Dudd = "Doodle"
        GoTo Escape
    End With
Escape:
End Sub

Private Function Doo() As String
    With Fudd
        .Dudd = "Doo"
        Doo = .Dudd
        Exit Function
    End With
    Unload Me 'Never executed.
End Function

Private Sub Form_Load()
    Diddle
    Doodle
    MsgBox Doo & vbNewLine & Fudd.Dudd
End Sub

But yes, there are special scenarios where GoTo or even GoSub can be useful. Things like GDI32 cleanup at the end of a procedure can often benefit from a "ladder" of labels that error conditions might return through to clean up created objects. Too bad Windows never got reference-counted "smart handles" for that sort of thing, but that would add more overhead.

[dilettante]

Of course that sent my mind to this topic:

Why are HANDLE return values so inconsistent?

If you look at the various functions that return HANDLEs, you’ll see that some of them return NULL (like CreateThread) and some of them return INVALID_HANDLE_VALUE (like CreateFile). You have to check the documentation to see what each particular function returns on failure.

Why are the return values so inconsistent?

The reasons, as you may suspect, are historical.

It is what it is though. No point in spitting into the wind.

[Niya]

Blockexit clears any dangling temporary locked pointer from abuse of With. As far as I can recall forgetting how With works is the only real hazard. Better yet, avoid using UDTs as much as possible.

I'd be real curious to hear what the trick has to say about this. He has intimate knowledge of the VB6 compiler and he probably knows exactly how With blocks work in VB6.

But yes, there are special scenarios where GoTo or even GoSub can be useful. Things like GDI32 cleanup at the end of a procedure can often benefit from a "ladder" of labels that error conditions might return through to clean up created objects.

I covered this earlier in this thread. Modern languages use structure exception handling and finally blocks for these scenarios. I think VB6 predates these constructs which is why the language doesn't have them. I mean even a prolific language like C doesn't have a standardize form of structured exception handling. Ancient languages like C and VB6's ancestral line suffer from this lack.

Too bad Windows never got reference-counted "smart handles" for that sort of thing, but that would add more overhead.

It would complicate accessing the Windows API because these kinds of things have to be created by convention. There would have to be a contract that compilers would all have to honor. For example, calling conventions are contracts that compilers have to honor if they want to call Windows API functions without problems. Smart handles would demand contracts of their own. My guess is that they wanted to keep it as simple as possible. We can then build our own systems on top of this if we wanted. For example COM is really just a system of contracts and protocols that both clients and servers have to honor.

[Niya]

Of course that sent my mind to this topic:

Why are HANDLE return values so inconsistent?



It is what it is though. No point in spitting into the wind.

There's a guy called Dave's Garage I'm subscribed to on YouTube. He is a retired programmer that worked on Windows during the Windows NT era. Between listening to him talk and reading Raymond Chen's blog, I often get the sense that Windows was developed under "the left hand doesn't know what the right hand is doing" kinds of conditions. A lot of times they would just invent things on the spot to support various things they were doing and often they would end up with different ways of doing the same thing.

[baka]

well. I know for sure this is happening for me, since I got the error.
but its hard to recreate. its a big program, so maybe it also depends on the complexity and speed.
if I try with just a few functions, nothing happens. I think there some kind of "refresh" that takes a bit to do, and if the program is too fast and too big it will not be able to clean it before I call it.
so something behind the scene that is doing all those lock/unlock, or free-up or whatever.
remember Im creating a game and the speed is 60+ frames per second that do tons of calls.

I know this because I get "new" errors depending on the size. before it worked, but now it will not. so I need to be very careful how I code. it need to be perfect.
and I notice that I need to close with block or it "can" crash.

[Niya]

Yea, it's probably best you continue to use GoTo in your With blocks. As programmers that need to provide software that people depend on in a timely manner we don't get the luxury of always knowing why. We just have to make it work.