2

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!

6
  • 2
    If you're looping and deleting, you should loop from right to left.
    – BigBen
    Jan 29, 2020 at 20:42
  • How would I do this?
    – BevoMG
    Jan 29, 2020 at 20:45
  • 5
    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. Jan 29, 2020 at 20:48
  • 1
    @BigBen me too. Typed it out dozens of times for rows, it's the exact same song & dance for columns. here's one Jan 29, 2020 at 20:50
  • 1
    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, 2020 at 20:58

1 Answer 1

12

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
4
  • 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....). 🙂 Jan 29, 2020 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. Jan 29, 2020 at 21:44
  • 1
    @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. Jan 29, 2020 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! Jan 29, 2020 at 23:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.