Improve VBA Performance

Question

I have a VBA code, which is used to iterate through the sorted data of Case IDs and transposes the row to the matching row if they are the same.

There are about 20k rows in the spreadsheet to look through. It often takes 20-40 minutes for the entire code to run. I'm not sure what I'm doing wrong.

Sub MyCombineRows()


    Dim r As Long
    Dim lngRow As Long
    Dim lngCol As Long
    Dim LastColumn As Long
    Dim sht As Worksheet
    Set sht = ActiveSheet
    'Application.ScreenUpdating = False

'   Set first row to start on (skipping first row of data)
    r = 3
    lngRow = sht.Cells(sht.Rows.Count, "A").End(xlUp).Row
    LastColumn = findLastCol(r - 1)

    Do
'       Check to see if columns A is equal to row above it
        If (Cells(r, "A") = Cells(r - 1, "A") And Cells(r, "A").Value <> "") Then
'           Copy value from column to end of row above it
            Range(Cells(r, 1), Cells(r, LastColumn)).Select
            Selection.Cut
            Cells(r - 1, LastColumn + 1).Select
            ActiveSheet.Paste
           'Delete Row
            Rows(r).Delete
            Do
                If (Cells(r, "A") = Cells(r - 1, "A") And Cells(r, "A").Value <> "") Then
                    Dim newLastCol As Long
                    newLastCol = findLastCol(r - 1)
                    Range(Cells(r, 1), Cells(r, LastColumn)).Select
                    Selection.Cut
                    Cells(r - 1, newLastCol + 1).Select
                    ActiveSheet.Paste
                    Rows(r).Delete
                Else
                    r = r + 1
                    If Cells(r, "A").Value = "" Then
                        Exit Do
                    End If
                End If
            Loop Until r = lngRow
        Else
'           Move on to next row
            r = r + 1
        End If
    Loop Until r = lngRow


End Sub

Function findLastCol(rowNum As Long) As Long
    Dim sht As Worksheet
    Set sht = ActiveSheet
    findLastCol = sht.Cells(rowNum, sht.Columns.Count).End(xlToLeft).Column
End Function

Use variant arrays to store and loop. Limit the number of times vba must reference the sheet. — Scott Craner, Jun 20 '19 at 15:46
It always helps to use `Application.ScreenUpdating = False` at teh beginning of your code (and reset it back to `True` at the end). Also, in general, you want to [avoid using select](https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba) in your code — cybernetic.nomad, Jun 20 '19 at 15:47
Also don't use copy and paste but rather set and read the .value parameter of your range objects — Dan, Jun 20 '19 at 15:47
Interacting with the applications "physical" objects is the reason why its awful. Its simply put as - for each iteration of your process you have to do the processing and then send the results back to the applications which must process your request and then apply it to the object and then send control back to your code. This behavior tends ot have exponential effects depending on how much youre expecting the application to change its' "physical" items/objects/components. TL;DR - dont play with the applications objects as much as possible. — Doug Coats, Jun 20 '19 at 15:49
I'd use a [dictionary](http://www.snb-vba.eu/VBA_Dictionary_en.html) to store the ID's with their index on the array to avoid matching (time consuming) — Damian, Jun 20 '19 at 15:49

score 1 · Answer 1 · answered Jun 20 '19 at 15:48

1

It might be the delete that is slowing you down as it is trying to update the UI each time which is usually quite slow. Try Application.ScreenUpdating = False at the start of your code and then switch it to true again when you have finished.

Alternatively just mark for deletion with a flag and delete all rows where the flag is set at the end.

answered Jun 20 '19 at 15:48

Nathan Scott

46
4

2

`Application.Calculation` mode is likely more impactful than `ScreenUpdating`, and using this global application state to mask inefficient code is a way too common bad practice. The solution is to stop `Select`ing and clipboard-ing and only interact with worksheets when you need to interact with a worksheet. Toggling `Application.ScreenUpdating`/`Calculation`/`EnableEvents` does not magically make inefficient code work faster, it just makes Excel work less between worksheet calls that shouldn't even be there in the first place. – Mathieu Guindon Jun 20 '19 at 16:02
1

As for the "mark for deletion" part, that's also wrong, because it introduces *even more* worksheet interactions. Marking for deletion is done by `Union`-ing the rows to be deleted, and then deleting that combined range - in one single worksheet operation; when there's only one worksheet operation happening, Calculation/ScreenUpdating/EnableEvent toggles make little to no difference. – Mathieu Guindon Jun 20 '19 at 16:06

Improve VBA Performance

1 Answers1