For Loop not fully cycling in excel VBA

Question

I have a macro where I search for text in a row and if a column does not have my specified text it is deleted. Here is my code:

Private Sub Test()

Dim lColumn As Long
    lColumn = ActiveSheet.Cells(2, Columns.Count).End(xlToLeft).Column


Dim i As Long
Dim myCell As Range
Dim myRange As Range
Set myRange = Worksheets("2019").Range(Cells(2, 1), Cells(2, lColumn))

For Each myCell In myRange
  If Not myCell Like "*($'000s)*" And Not myCell Like "*Stmt Entry*" And Not myCell Like "*TCF*" And_ 
  Not myCell Like "*Subtotal*" And Not myCell Like "*Hold*" Then
    myCell.EntireColumn.Select
    Selection.Delete
  End If

Next

End Sub

My issue is that when I execute the macro it will only delete some of the columns but not the ones towards the end of the range. If I then run the macro again it will successfully delete all the columns I ask it to.

If I switch the macro to- let's say- make the cells bold instead of deleting them it works perfectly every time.

What am I missing?

Many thanks!

If you're looping and deleting, you should loop from right to left. — BigBen, Jan 29 '20 at 20:42
You're modifying a collection as you're iterating it. This usually means unexpected, weird behavior. Consider `Union`-ing the columns instead of `Select`-ing them, and then run the `.Delete` method of the unioned `Range` object *once, **after** the loop* instead of deleting the `Selection` at every iteration. Looping backwards to keep inefficiently modifying the collection you're iterating is just.... backwards. — Mathieu Guindon, Jan 29 '20 at 20:48
@BigBen me too. Typed it out dozens of times for rows, it's the exact same song & dance for columns. [here's one](https://stackoverflow.com/a/47873216/1188513) — Mathieu Guindon, Jan 29 '20 at 20:50
Suggest you don't use ActiveSheet to get the last column; use the sheet you are actually interested in. There is no guarantee that the ActiveSheet will be what you expect. — SmileyFtW, Jan 29 '20 at 20:58

score 12 · Answer 1 · answered Jan 29 '20 at 21:00

Despite everyone saying "just loop backwards" in this & linked posts, that's not what you want to do.

It's going to work, and then your next question will be "how can I speed up this loop".

The real solution is to stop what you're doing, and do things differently. Modifying a collection as you're iterating it is never a good idea.

Start with a helper function that can combine two ranges into one:

Private Function CombineRanges(ByVal source As Range, ByVal toCombine As Range) As Range
    If source Is Nothing Then
        'note: returns Nothing if toCombine is Nothing
        Set CombineRanges = toCombine
    Else
        Set CombineRanges = Union(source, toCombine)
    End If
End Function

Then declare a toDelete range and use this CombineRanges function to build ("select") a Range while you're iterating - note that this loop does not modify any cells anywhere:

Dim sheet As Worksheet
' todo: use sheet's codename instead if '2019' is in ThisWorkbook
Set sheet = ActiveWorkbook.Worksheets("2019")

Dim source As Range
' note: qualified .Cells member calls refer to same sheet as .Range call
Set source = sheet.Range(sheet.Cells(2, 1), sheet.Cells(2, lColumn))

Dim toDelete As Range
Dim cell As Range
For Each cell In source
    'note: needed because comparing cell.Value with anything will throw error 13 "type mismatch" if cell contains a worksheet error value.
    'alternatively, use cell.Text.
    If Not IsError(cell.Value) Then
        If Not cell.Value Like "*($'000s)*" _
            And Not cell.Value Like "*Stmt Entry*" _
            And Not cell.Value Like "*TCF*" _
            And Not cell.Value Like "*Subtotal*" _
            And Not cell.Value Like "*Hold*" _
        Then
            Set toDelete = CombineRanges(cell, toDelete)
        End If
    End If
Next

The last, final step is to delete the .EntireColumn of the toDelete range... if it isn't Nothing at that point:

If Not toDelete Is Nothing Then toDelete.EntireColumn.Delete

INCONCEIVABLE! How does this answer have 4 likes while my near same-result answer has zero, even though my answer was a whopping two minutes earlier. This is all BigBen's doing -- calling my code a HACK. Because I have nothing better to do, I'm going to add a fifth upvote to this answer because I'm trying to earn the stackoverflow, "*you're a good freaking sport trophy by voting for competing answers*" More seriously... nice defense at end for `If not is nothing Then xx.Delete`. I didn't have that (which is probably why I have less than 10% of your SO street cred....). — pgSystemTester, Jan 29 '20 at 21:39
Okay either my mother is back on StackOverflow or somebody must have read me whining and gave me a like. **THANKS!** *Serious Question to anyone reputable:* See my approach of creating the last/row column approach to avoid an if statement on each loop. Thoughts on if this is better/worse/too insignificant to worth worrying about? Thanks. — pgSystemTester, Jan 29 '20 at 21:44
@PGSystemTester the initial assignment of `killrng` to the very last column on the sheet to ensure `killrng` isn't `Nothing`, is extraneous work that doesn't need to happen with a null check. While it's indeed unlikely that there's anything of value in that column, macros shouldn't affect cells that have no reason to be affected: using that "hack" means baking an assumption into the code, and any assumption that can be *removed* from the code *should* be removed from the code. — Mathieu Guindon, Jan 29 '20 at 21:52
Thanks. I appreciate the feedback. I could live with the possibility of problem on the last row, however I thought I was gaining some improvement with speed. I ran some tests.... no significant improvement at all. So I agree. Thanks for feedback! — pgSystemTester, Jan 29 '20 at 23:54

For Loop not fully cycling in excel VBA

1 Answers1

Linked

Related