VBA Comparing from two sheets is too slow?

Question

So here is my code , im trying to compare a sheet containing 3000 rows with another one of 5000 rows, however it is working too slow, can anyone help ?

Dim G As Long
Dim K As Long
Dim CardBrand As String
Dim STD As String
Dim CardBrand2 As String
Dim ASI
Dim ID As String
Dim X As Workbook
Dim FinalRow As Long
Dim Finalrow2 As Long
Dim I As Long
Dim TC_STD As String
Dim TC_ASI As String
Dim TC_Perc As Double
Dim TC_Base As Double
Dim TC_ID As String


    Application.ScreenUpdating = False
    FinalRow = Cells(Rows.Count, "I").End(xlUp).Row


    For G = 5 To FinalRow

        CardBrand = Sheets("sheet1").Cells(G, 9).Value
        STD = Sheets("sheet1").Cells(G, 10).Value
        ID = Sheets("sheet1").Cells(G, 5).Value

        For K = 2 To 51

            CardBrand2 = Sheets("sheet2").Cells(K, 3).Value

            If CardBrand = CardBrand2 Then

                ASI = Sheets("sheet2").Cells(K, 1).Value
                Set X = Workbooks.Open("E:\Partner_Commission_Compiler\Repository\Transaction_Charges.xlsx")
                Finalrow2 = X.ActiveSheet.Cells(Rows.Count, "A").End(xlUp).Row

                For I = 1 To Finalrow2

                    TC_ASI = X.ActiveSheet.Cells(I, 6).Value
                    TC_STD = X.ActiveSheet.Cells(I, 11).Value
                    TC_ID = X.ActiveSheet.Cells(I, 1).Value

                        If (TC_ASI = ASI) And (TC_STD = STD Or TC_STD = "All") And TC_ID = ID Then

                            TC_Perc = X.ActiveSheet.Cells(I, 19).Value
                            TC_Base = X.ActiveSheet.Cells(I, 20).Value
                            ThisWorkbook1.Sheets("Sheet1").Activate

                            Sheets("sheet1").Cells(G, 13).Value = TC_Perc
                            Sheets("sheet1").Cells(G, 14).Value = TC_Base
                        End If
                Next I
            End If
        Next K
    Next G

    X.Close (False)
    Application.ScreenUpdating = True

Your code would be so much easier to read and understand if it was indented. I can't be bothered doing it so it's up to you. — Mark Fitzgerald, Mar 25 '15 at 10:51
Hi Mark, iv updated it, however indentation is not coming as it should be for a reason or another. — Kurt Desira, Mar 25 '15 at 11:03
It would be a big change for you but any code that loops on a lot of cells can take quite a while, so in this scenario I always use arrays. Basic concept is that each iteration of a loop reading a cell is effectively an I/O between the software layers of the code and the worksheet, so more cells = more i/o. Is you use an array method, you read the contents of both sheets into an array (1 each) = 2 I/O's - then do what ever you want in the code, maybe writing out to another array or updating the existing ones, and at the end, write any updated or new array back to your target. 3 I/O's — Mark Moore, Mar 25 '15 at 11:08
As your code is working, albeit slowly, your question may get more answers on [Code review](http://codereview.stackexchange.com/) — Mark Fitzgerald, Mar 25 '15 at 11:37
possible duplicate of [VBA Excel large data manipulation getting memory issue](http://stackoverflow.com/questions/6134002/vba-excel-large-data-manipulation-getting-memory-issue). Also very related: http://stackoverflow.com/questions/5387929/vba-macro-to-compare-all-cells-of-two-excel-files — Jean-François Corbett, Mar 25 '15 at 11:51

cybermike · Accepted Answer · 2015-03-25T12:09:39.890

Some suggestions:

Since you are suppressing ScreenUpdating, and you are (quite correctly) not using copy and paste to move the data from X.ActiveSheet to Sheet1, then you really don't need the line "ThisWorkbook1.Sheets("Sheet1").Activate" repeating over and over in the loop. An Activate call can be quite time consuming even when Sheet1 is already activated, and its not like you are flipping which sheet is active.
Also, you are re-opening "E:\Partner_Commission_Compiler\Repository\Transaction_Charges.xlsx" over and over in the loop. Again needless, and without a doubt is consuming more and more CPU time when you do not need to.

The following two lines should be moved before any of your loops:

Set X = Workbooks.Open("E:\Partner_Commission_Compiler\Repository\Transaction_Charges.xlsx")
Finalrow2 = X.ActiveSheet.Cells(Rows.Count, "A").End(xlUp).Row

thanks very helpful ! saved me a lot of time :) – Kurt Desira Mar 25 '15 at 18:31 — Kurt Desira, Mar 25 '15 at 18:31

score 1 · Answer 2 · answered Mar 25 '15 at 13:22

In addition to cybermike's fine suggestions (not opening the file on every loop should save significant time), you could try these changes.

Change this:

CardBrand2 = Sheets("sheet2").Cells(K, 3).Value
If CardBrand = CardBrand2 Then

to this

If Sheets("sheet2").Cells(K, 3) = Sheets("sheet2").Cells(K, 3).Value

You have:

Dim ASI

Which declares it a Variant. Every time your code uses ASI, Excel has to decipher what type of data is stored in ASI to determine how to assign or compare it. If you declare it a specific type, it can skip that determination step which will speed up execution. Since you're assigning it to the contents of a cell, you could probably specify String or Integer. If it's sometimes one, sometimes the other, Dim as String, then explicitly CStr(cell).value and do all your comparisons as strings. It will again eliminate time needed for Excel to figure out what to do with the values.

You can replace:

TC_ASI = X.ActiveSheet.Cells(I, 6).Value
TC_STD = X.ActiveSheet.Cells(I, 11).Value
TC_ID = X.ActiveSheet.Cells(I, 1).Value
If (TC_ASI = ASI) And (TC_STD = STD Or TC_STD = "All") And TC_ID = ID Then
  TC_Perc = X.ActiveSheet.Cells(I, 19).Value
  TC_Base = X.ActiveSheet.Cells(I, 20).Value
  ThisWorkbook1.Sheets("Sheet1").Activate
  Sheets("sheet1").Cells(G, 13).Value = TC_Perc
  Sheets("sheet1").Cells(G, 14).Value = TC_Base

with:

If x.cells(i,6) = Sheets("sheet2").Cells(K, 1) AND _
   (x.cells(1,11) = Sheets("sheet1").Cells(G, 10)  OR _
    x.cells(1,11) = "All") AND _
   x.Cells(i,1) = Sheets("sheet1").Cells(G, 5) Then
  Sheets("sheet1").Cells(G, 13).Value = X.Cells(I, 19)
  Sheets("sheet1").Cells(G, 14).Value = X.Cells(I, 20)

Removing all those assignments on each loop will save some processing time. It's a bit harder to read the code, though, so you may want to leave some pseudocode in comments to help remember what all those different cells represent.

VBA Comparing from two sheets is too slow?

2 Answers2