5

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.

Improve this question
4
  • stackoverflow.com/questions/19234328/… Commented Oct 8, 2013 at 8:27
  • +1 for an interesting set of questions. The question of "why don't they get deleted has been answered. Commented Oct 8, 2013 at 10:38
  • 2
    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. Commented Oct 9, 2013 at 15:39
  • Autofilter code added. Commented Oct 9, 2013 at 15:53

4 Answers 4

Reset to default
4

This is the worst way to delete a row. Reasons

  1. You are deleting the rows in a Loop
  2. 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.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

1. Are loops necessarily bad?
2. If I activate the worksheet just before I execute the code, does it then still matter that the cells object is not qualified?
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 :)
Regarding Point 2. It won't matter but still it is good to qualify your objects. INTERESTING READ
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 ;)
4

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
Improve this answer

1 Comment

+ 1 Even my fav :) The fastest way (a better alternative to loops!)
3

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
Improve this answer

2 Comments

Are you saying that it is always better to delete from the bottom because then you don't run the risk of skipping rows?
@Anonymous Yes, exactly.
2

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
Improve this answer

5 Comments

Thanks for pointing the algorithmic mistake out - still learning over here ;-)
This works but is too slow to be practical - I will try one of the other suggested answers. Thanks though!
I have no doubts about it. I just corrected your code (and answered your question!), not optimized it!
@Anonymous: And hence my post ;) But I agree with LC_dev as well. He answered your question. :)
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!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.