VBa conditional delete loop not working

Question

I am running the following code on a spreadsheet:

Do While i <= 100000
    If Not Cells(i, 4) = "String" Then
        Cells(i, 4).EntireRow.Delete
    End If
    i = i + 1
Loop

There are plenty of entries with not "String" but they do not get deleted.

When I copy this piece of code to a separate sheet, I even get the error "Excel cannot complete this task with available resources. Choose less data or close other applications."

What am I doing wrong that is making this loop not work?

Note: I can't use autofilter because I need to delete rows based on not meeting a condition.

http://stackoverflow.com/questions/19234328/if-single-cell-value-is-found-in-a-range-then-delete-entire-row — , Oct 08 '13 at 08:27
+1 for an interesting set of questions. The question of "why don't they get deleted has been answered. — Mark Fitzgerald, Oct 08 '13 at 10:38
actualy you can use AutoFilter directly. And even if you couldnt you can insert a working column and run on that. Which is much superior to range looping. — brettdj, Oct 09 '13 at 15:39

Siddharth Rout · Answer 1 · 2013-10-08T08:12:02.763

4

This is the worst way to delete a row. Reasons

You are deleting the rows in a Loop
Your Cells Object are not qualified

Try this.

Co-incidentally I answered a similar question in the MSDN forum as well. Please See THIS

Try this way (UNTESTED)

In the below code I have hardcoded the last row to 100000 unlike as done in the above link.

Sub Sample()
    Dim ws As Worksheet
    Dim i As Long
    Dim delRange As Range

    '~~> Set this to the relevant worksheet
    Set ws = ThisWorkbook.Sheets("Sheet1")

    With ws
        For i = 1 To 100000
            If .Cells(i, 4).Value <> "String" Then
                If delRange Is Nothing Then
                    Set delRange = .Rows(i)
                Else
                    Set delRange = Union(delRange, .Rows(i))
                End If
            End If
        Next i

        If Not delRange Is Nothing Then delRange.Delete
    End With
End Sub

NOTE: I am assuming that a cell will have values like

String
aaa
bbb
ccc
String

If you have scenarios where the "String" can be in different cases or in between other strings for example

String
aaa
STRING
ccc
dddStringddd

then you will have to take a slightly different approach as shown in that link.

edited Oct 08 '13 at 08:12

answered Oct 08 '13 at 07:47

Siddharth Rout

147,039
17
206
250

2

1. Are loops necessarily bad? – Aaa Oct 08 '13 at 08:01
1

2. If I activate the worksheet just before I execute the code, does it then still matter that the cells object is not qualified? – Aaa Oct 08 '13 at 08:02
2

No they are not. In fact I am using a loop :) But when you delete rows in a loop then your code becomes very slow :) – Siddharth Rout Oct 08 '13 at 08:02
1

Regarding Point 2. It won't matter but still it is good to qualify your objects. [INTERESTING READ](http://stackoverflow.com/questions/10714251/excel-macro-avoiding-using-select) – Siddharth Rout Oct 08 '13 at 08:03
1

Also when u delete rows outside the loop like I have done then it really doesn't matter if you loop from top to bottom or bottom to top ;) – Siddharth Rout Oct 08 '13 at 08:09

score 4 · Answer 2 · edited Oct 09 '13 at 17:03

Autofilter code:

Sub QuickCull()
    Dim rng1 As Range

    Set rng1 = Range([d4], Cells(Rows.Count, "D").End(xlUp))
    ActiveSheet.AutoFilterMode = False

    With Application
        .DisplayAlerts = False
        .ScreenUpdating = False
    End With

    With rng1
        .AutoFilter Field:=1, Criteria1:="<>string"
        If rng1.SpecialCells(xlCellTypeVisible).Count > 1 Then _
        .Offset(1, 0).Resize(rng1.Rows.Count - 1).Rows.Delete
    End With

    With Application
        .DisplayAlerts = True
        .ScreenUpdating = True
    End With

    ActiveSheet.AutoFilterMode = False
End Sub

+ 1 Even my fav :) The fastest way (a better alternative to loops!) — Siddharth Rout, Oct 09 '13 at 17:01

score 3 · Answer 3 · answered Oct 08 '13 at 07:50

When you want to delete rows its always better to delete from bottom.

Sub DeleteData()

    Dim r As Long
    Dim Rng As Range

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual

    With ThisWorkbook.Sheets("sheet1")

        Set Rng = .Range(.Range("D1"), .Range("D1").End(xlDown))

        For r = Rng.Rows.Count To 1 Step -1
            If LCase(Trim(.Cells(r, 4).Value)) <> LCase("string") Then
                .Cells(r, 4).EntireRow.Delete
            End If
        Next

    End With

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic

End Sub

Are you saying that it is *always* better to delete from the bottom because then you don't run the risk of skipping rows? — Aaa, Oct 08 '13 at 07:58

score 1 · Accepted Answer · answered Oct 08 '13 at 07:49

1

This is a basic algorithm mistake.

Imagine your program are on, say, row 10. You delete it. So, row 11 becomes row 10, row 12 becomes 11 and so on. Then you go to row 11, skipping row 10, previous row 11!

This would work:

Do While i <= 100000
    If Not Cells(i, 4) = "String" Then
        Cells(i, 4).EntireRow.Delete
    Else
        i = i + 1
    End If
Loop

answered Oct 08 '13 at 07:49

LS_ᴅᴇᴠ

10,823
1
23
46

Thanks for pointing the algorithmic mistake out - still learning over here ;-) – Aaa Oct 08 '13 at 08:16
1

This works but is too slow to be practical - I will try one of the other suggested answers. Thanks though! – Aaa Oct 08 '13 at 08:45
3

I have no doubts about it. I just corrected your code (and answered your question!), not optimized it! – LS_ᴅᴇᴠ Oct 08 '13 at 08:47
3

@Anonymous: And hence my post ;) But I agree with LC_dev as well. He answered your question. :) – Siddharth Rout Oct 08 '13 at 10:54
Thanks to the other contributers too - I ended up going with Siddharth's solution but it's true, LS_dev explained best why my code was broken and corrected it. Thank you guys for being so helpful! – Aaa Oct 08 '13 at 11:08

VBa conditional delete loop not working

4 Answers4

Linked

Related