I am using an if statement nested in a loop that is slowing down my code. What can I do to speed it up?

Question

I have an if statement nested in a loop that I am using to clean up imported data. The if statement evaluates the value of the active cell and then deletes the row of the active cell if it meets certain criteria. I'm wondering if there's another way to code this so it's not referencing the spreadsheet for every iteration, and consequently making it run faster than it currently is. Any tips would be appreciated. Code I am using is below:

Sub copy_RawAvgDem()

Dim wkb1 As Workbook
Dim sht1 As Worksheet
Dim wkb2 As Workbook
Dim sht2 As Worksheet

Set wkb1 = ThisWorkbook
Set wkb2 = Workbooks.Open("M:\FAST team\Inventory_Planning\2016_05_FG_Inv_targets.xlsx")
Set sht1 = wkb1.Sheets("RawAvgDem")
Set sht2 = wkb2.Sheets("Model")

sht2.ShowAllData
sht2.Cells.Copy
sht1.Range("A1").PasteSpecial xlPasteValues
Application.CutCopyMode = False
wkb2.Close False

Worksheets("RawAvgDem").Activate
Range("AN2").Select

Do Until IsEmpty(ActiveCell.Value)
    If ActiveCell.Value = "MTO" Then
        Rows(ActiveCell.Row).EntireRow.Delete
    Else: ActiveCell.Offset(1, 0).Select
    End If
Loop

End Sub

When you have wkb2 open instead of reading data to the sheet read this data into an array and then remove any "rows" from the array then output the array onto the sheet. Calling the sheet is expensive (in terms of running time) and should be avoided if speed is needed — 99moorem, Jun 19 '17 at 14:32
On a side note: Read https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros - Why not to select and activate. As to your loop... Try using `AutoFilter` to filter all the rows with "MTO" in it and delete them at once. — Rik Sportel, Jun 19 '17 at 14:34

Soulfire · Answer 1 · 2017-06-19T15:04:08.880

The best way to improve performance is to eliminate loops if possible. As alluded to in the comments, you could filter column AN on the appropriate value and then remove the rows all at once rather than loop through. After wkb2.Close you could do this instead:

With Worksheets("RawAvgDem")
        .Range("$AN$2").AutoFilter Field:=Range("$AN$2").Column, Criteria1:="MTO"
        .Range(Range("$AN$3"), Range("$AN$3").End(xlDown)).EntireRow.Delete
        .Range("$AN$2").AutoFilter Field:=Range("$AN$2").Column
End With

This assumes that there is data in columns A-AN. If this is not the case, you'd have to update the Field to the appropriate number (this is a relative number based on the number of columns filtered). For reference, AN is the 40th column in the spreadsheet (Range("$AN$2").Column returns 40, so a static 40 would also work there). If you were missing data in Column A, for example, this number would have to be 39. Adjust as needed.

score 0 · Answer 2 · answered Jun 19 '17 at 14:33

0

You don't need to use ActiveCell.Offset(1, 0).Select to advance to the next row, replace your loop at the end with the loop below:

With Worksheets("RawAvgDem")
    Dim i As Long

    i = 2
    Do Until IsEmpty(.Range("AN" & i).Value)
        If .Range("AN" & i).Value = "MTO" Then
            .Rows(i).Delete
        Else
            i = i + 1
        End If
    Loop
End With

answered Jun 19 '17 at 14:33

Shai Rado

33,032
6
29
51

2

To expand slightly: in general, one should always avoid using `Select` or `Activate`. – Soulfire Jun 19 '17 at 14:37
@Soulfire that's true, I just wanted to focus the PO where he should make the update – Shai Rado Jun 19 '17 at 14:54
also, i would do backward with i. You might miss rows to delete: say row 4 and 5 are to delete. But after deleting row4, you go to row 5 but in fact you are already at 6 because deleting 4 makes 5 become 4 – Patrick Lepelletier Jun 20 '17 at 00:41
@PatrickLepelletier actually no, because you are advancing the row counter i, only when not deleting a row, check the code logics again – Shai Rado Jun 20 '17 at 01:47

I am using an if statement nested in a loop that is slowing down my code. What can I do to speed it up?

2 Answers2