none
For Each statement is running really slow RRS feed

  • Question

  • I have to improve this code somehow in speed. I know the problem is in the For Each statement but I really don't know where to start to fix it.

    Private Sub chkAuditDates()
        'Set path to audits (NETWORK)
        Const FolderPath As String = "\\JACKSONVILLE-DC\Common\SOP's for JV\SOP Audits\2019"
        'Set path to audits (LOCAL)
        'Const FolderPath As String = "C:\Users\jbishop\Desktop\SOP Audits with New Names"
        
        'Instantiate the FSO & related vars
        Dim oFSO As Object: Set oFSO = CreateObject("Scripting.FileSystemObject")
        Dim oFolder As Object: Set oFolder = oFSO.GetFolder(FolderPath)
        Dim oFiles As Object: Set oFiles = oFolder.Files
        Dim oFile As Object
            
        Dim i As Integer
        'Loop through all worksheets - NEED TO ESTABLISH LOOP/CURRENTLY SET TO ONE SHEET
        'For i = 1 To 12
            With Worksheets(3)
                'Set cell background color to Red for a range of cells
                With Range("E1:P" & .Cells(.Rows.Count, 1).End(xlUp).Row)
                    '.Interior.Color = RGB(255, 0, 0)
                    .HorizontalAlignment = xlCenter
                    .Font.Color = vbBlack
                    .Font.Bold = True
                End With
                
                'Store cells in COL A that have values as a range
                Dim SOPID As Range: Set SOPID = .Range("A1", .Range("A" & .Rows.Count).End(xlUp))
                Dim cel As Range
                
                'Loop through each SOP audit file
                For Each oFile In oFiles
                    'Strip audit date out of filename and trim off the file extension
                    Dim auditDate: auditDate = CDate(DateSerial(Right(Left(Split(oFile.Name, "-")(3), 8), 4), _
                                                                Left(Left(Split(oFile.Name, "-")(3), 8), 2), _
                                                                Mid(Left(Split(oFile.Name, "-")(3), 8), 3, 2)))
                    
                    'Loop through all SOP IDs stored in COL A
                    For Each cel In SOPID
                        
                        'See if SOP ID in COL A matches SOP ID in Audit file name
                        If Trim(RemoveLeadingZeroes(Split(oFile.Name, "-")(2))) = Trim(cel) Then
                            'Insert link to audit, change background color, etc of selected cell
                            With cel.Offset(0, 3 + Month(auditDate))
                                .Hyperlinks.Add Anchor:=cel.Offset(0, 3 + Month(auditDate)), Address:=oFile.Path, TextToDisplay:="X"
                                .Interior.Color = RGB(34, 139, 34)
                                .Font.Color = vbBlack
                                .Font.Bold = True
                            End With
                        End If
                        
                    Next cel
                Next oFile
                
                'Autosize columns to best fit inserted data
                .Columns("A:P").AutoFit
            End With
        'Next i
    End Sub

    It freezes excel if I run it on all 12 sheets. If I change the code and manually tell it an individual sheet to run it on, it works but it is slow.

    I don't understand the differences between running a For Each vs a For on a range or variant or cells. I need some enlightenment badly.


    Friday, July 26, 2019 3:18 PM

Answers

  • Dim SOPID As Variant: SOPID = .Range("A1", Range("A" & .Rows.Count).End(xlUp))

    That's the problem row. Biggest problem is .Rows.Count. Because of the With statement. it is the same as Worksheets(3).rows.count which is 1048576.

    So ideally your worksheet has one row of titles only and a blank column either side (or ColA) and a blank row below. That way a Ctrl+* highlights all your data.

    In VBA I would use:

    Dim Rng as range

    Set Rng=worksheets(3).Range("A1")

    For each cel in rng.currentregion

    looks at each cell in your data - same range as Ctrl+*

    This should be massively faster!

    Out of interest, in Excel 2016 in a new, blank workbook I ran the following code:

    Sub Test()
    Dim Rng As Range
    Dim Row As Long
    Dim Col As Long
    Dim MaxRow As Long
    Dim MaxCol As Long
    Dim Tim As Single
        Range("A1:Z30000").Formula = 1
       
        'Test 1 For Each
        Tim = Timer
        For Each Rng In Range("A1").CurrentRegion
            Rng = Rng + 1
        Next Rng
        Debug.Print "For Each loop: " & Format(Timer - Tim, "0\s")

        'Test 2 Cells loop
        Tim = Timer
        MaxRow = Range("A1").CurrentRegion.Rows.Count
        MaxCol = Range("A1").CurrentRegion.Columns.Count
        For Row = 1 To MaxRow
            For Col = 1 To MaxCol
                Set Rng = Cells(Row, Col)
                Rng = Rng + 1
            Next Col
        Next Row
        Debug.Print "Cells loop: " & Format(Timer - Tim, "0\s")
    End Sub

    Results in the Immediate window were:

    For Each loop: 18s
    Cells loop: 20sSo nothing wrong with the For Each speed :-)


    Rod Gill
    Author of the one and only Project VBA Book and VBA developer.
    www.project-systems.co.nz

    • Marked as answer by mongoose00318 Thursday, August 15, 2019 5:49 PM
    Saturday, July 27, 2019 12:12 AM

All replies

  • I think I almost have it working...

    How can I tell this statement to ignore the first row (A1)?

    Dim SOPID As Variant: SOPID = .Range("A1", Range("A" & .Rows.Count).End(xlUp))

    I've tried changing it to .Range("A2"...but it still shows the value of cell A1 in the watch window.
    Friday, July 26, 2019 4:06 PM
  • Dim SOPID As Variant: SOPID = .Range("A1", Range("A" & .Rows.Count).End(xlUp))

    That's the problem row. Biggest problem is .Rows.Count. Because of the With statement. it is the same as Worksheets(3).rows.count which is 1048576.

    So ideally your worksheet has one row of titles only and a blank column either side (or ColA) and a blank row below. That way a Ctrl+* highlights all your data.

    In VBA I would use:

    Dim Rng as range

    Set Rng=worksheets(3).Range("A1")

    For each cel in rng.currentregion

    looks at each cell in your data - same range as Ctrl+*

    This should be massively faster!

    Out of interest, in Excel 2016 in a new, blank workbook I ran the following code:

    Sub Test()
    Dim Rng As Range
    Dim Row As Long
    Dim Col As Long
    Dim MaxRow As Long
    Dim MaxCol As Long
    Dim Tim As Single
        Range("A1:Z30000").Formula = 1
       
        'Test 1 For Each
        Tim = Timer
        For Each Rng In Range("A1").CurrentRegion
            Rng = Rng + 1
        Next Rng
        Debug.Print "For Each loop: " & Format(Timer - Tim, "0\s")

        'Test 2 Cells loop
        Tim = Timer
        MaxRow = Range("A1").CurrentRegion.Rows.Count
        MaxCol = Range("A1").CurrentRegion.Columns.Count
        For Row = 1 To MaxRow
            For Col = 1 To MaxCol
                Set Rng = Cells(Row, Col)
                Rng = Rng + 1
            Next Col
        Next Row
        Debug.Print "Cells loop: " & Format(Timer - Tim, "0\s")
    End Sub

    Results in the Immediate window were:

    For Each loop: 18s
    Cells loop: 20sSo nothing wrong with the For Each speed :-)


    Rod Gill
    Author of the one and only Project VBA Book and VBA developer.
    www.project-systems.co.nz

    • Marked as answer by mongoose00318 Thursday, August 15, 2019 5:49 PM
    Saturday, July 27, 2019 12:12 AM