1
\$\begingroup\$

I am currently running multiple macros on a large volume of files however watching it go through its processes today I realise one of my issue is that my loop runs through my workbooks 3 times instead of actioning everything it needs to and then moving onto the next file.

This task goes through around 18 files that have been opened by another part of the macro. It runs as follows:

  1. Copies down formulas, calculates and pastes the results as values.
  2. Refreshes the pivots within the file and will copy anything in column "B" from one of these to another sheet.
  3. Copies down formulas and calculates on another worksheet

This is my first time doing a loop so I am obviously missing something to make this process faster.


Sub Update_Formulas_Loop()


    Dim WB As Workbook
    Dim WS As Worksheet
    Dim WS1 As Worksheet
    Dim WS2 As Worksheet
    Dim WS3 As Worksheet
    Dim Master As Workbook
    Dim LR As Long
    Dim LR1 As Long
    Dim LR2 As Long
    Dim LR3
    Dim Table As PivotTable
        
    Set Master = ThisWorkbook
    
    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.ErrorCheckingOptions.BackgroundChecking = False
    
    For Each WB In Application.Workbooks
        If WB.Name <> "Master File.xlsb" Then
            For Each WS In WB.Worksheets
                If WS.Name = "Worksheet" Then
                    WS.Activate
                    LR2 = WS.Cells(Rows.Count, "I").End(xlUp).Row
                    Range("Q2:S2").Copy
                    Range("Q4:S" & LR2).PasteSpecial xlPasteFormulas
                    Range("X2:AD2").Copy
                    Range("X4:AD" & LR2).PasteSpecial xlPasteFormulas
                    Range("AH2:AI2").Copy
                    Range("AH4:AI" & LR2).PasteSpecial xlPasteFormulas
                    Application.Calculate
                    Range("Q4:AD" & LR2).Copy
                    Range("Q4:AD" & LR2).PasteSpecial xlPasteValues
                    ActiveWorkbook.RefreshAll
               End If
            Next WS
    
            For Each WS1 In WB.Worksheets
                If WS1.Name = "VS TB" Then
                Sheets("Check Sheet").Activate
                Range(Range("B6").EntireRow, Range("B6").EntireRow.End(xlDown)).Delete
                WS1.Activate
                LR4 = WS1.Cells(Rows.Count, "B").End(xlUp).Row
                Range("B5:B" & LR4).Copy
                Sheets("Check Sheet").Activate
                Range("B5").PasteSpecial xlPasteValues
                WS1.Activate
            End If
            Next WS1
                
            For Each WS2 In WB.Worksheets
                If WS2.Name = "Check Sheet" Then
                WS2.Activate
                LR3 = WS2.Cells(Rows.Count, "B").End(xlUp).Row
                Range("C5:AC5").Copy
                Range("C5:AC" & LR3).PasteSpecial xlPasteFormulas
                Application.Calculate
                
            End If
            Next WS2
            End If
    Next WB
   
End Sub
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Aug 23, 2022 at 15:45
  • \$\begingroup\$ On the topic of speed: You can shave off a few milliseconds per loop by removing .Copy and directly assigning the values to the destination range like Range("Q4:S" & LR2).Formula = Range("Q2:S2").Formula \$\endgroup\$
    – Toddleson
    Aug 29, 2022 at 14:43

2 Answers 2

3
\$\begingroup\$

To me, it looks like the only loop you need is when you're checking all the open workbooks. Inside of the other loops, where you're running through each worksheet, you are specifically calling out the sheet you want. Therefore, a loop is not required at all. Other comments on your code:

  1. Try to avoid a "wall of declarations" for all your variables and declare them immediately before using them.
  2. Separate repetitive code into its own sub/function. In your case, you never re-enable all the Application level settings. In my example below I show an easy Sub that can be used. Which leads to...
  3. Try to make your code self-documenting. Name your variables in a way that helps the programmer identify the purpose of each set of logic. Supplement these logic blocks with a comment that explains what you're trying to do.
  4. Avoid using Activate or Select. It's actually very rare that you'll ever need these in VBA.
  5. Learning the With statement can make your code more efficient.
  6. You can re-use variables when it makes sense, i.e. lastRow in the example below.

Here is an example using these hints (NOTE- code is untested since I don't have your data):

Option Explicit

Sub UpdateWorkbookFormulas()
    AppSettings SetTo:=False
    
    '--- examine all currently open workbooks
    Dim openBook As Workbook
    For Each openBook In Application.Workbooks
        If openBook.Name <> "Master File.xlsb" Then
        
            '--- first refresh all the formulas to calculate the
            '    values, pasting only the values when done
            Dim lastRow As Long
            With openBook.Worksheets("Worksheet")
                lastRow = .Cells(.Rows.Count, "I").End(xlUp).Row
                .Range("Q2:S2").Copy
                .Range("Q4:S" & lastRow).PasteSpecial xlPasteFormulas
                .Range("X2:AD2").Copy
                .Range("X4:AD" & lastRow).PasteSpecial xlPasteFormulas
                .Range("AH2:AI2").Copy
                .Range("AH4:AI" & lastRow).PasteSpecial xlPasteFormulas
                Application.Calculate
                .Range("Q4:AD" & lastRow).Copy
                .Range("Q4:AD" & lastRow).PasteSpecial xlPasteValues
            End With
            openBook.RefreshAll

            '--- clear up the check sheet data ...
            Dim checkSheet As Worksheet
            Set checkSheet = openBook.Worksheets("Check Sheet")
            With checkSheet
                .Range(.Range("B6").EntireRow, .Range("B6").EntireRow.End(xlDown)).Delete
            End With
        
            '--- ... and copy in the updated values
            With openBook.Worksheets("VS TB")
                lastRow = .Cells(.Rows.Count, "B").End(xlUp).Row
                .Range("B5:B" & lastRow).Copy
            End With
            checkSheet.Range("B5").PasteSpecial xlPasteValues
    
            '--- finally update the check sheet with the formulas
            With checkSheet
                lastRow = .Cells(.Rows.Count, "B").End(xlUp).Row
                .Range("C5:AC5").Copy
                .Range("C5:AC" & LR3).PasteSpecial xlPasteFormulas
                Application.Calculate
            End With
        End If
    Next openBook
    AppSettings SetTo:=True
End Sub

Sub AppSettings(ByVal SetTo As Boolean)
    '--- this should really capture the current state of
    '    each setting, then restore it when you're done
    Application.Calculation = IIf(SetTo, xlCalculationAutomatic, _
                                         xlCalculationManual)
    Application.ScreenUpdating = SetTo
    Application.EnableEvents = SetTo
    Application.ErrorCheckingOptions.BackgroundChecking = SetTo
End Sub
\$\endgroup\$
2
\$\begingroup\$

Something you could do to further improve the code and in the spirit of producing self-documenting code: use named ranges.

Because ranges such as: Range("Q4:S" & lastRow) or: Range("X2:AD2") are not meaningful at all. One benefit is that you define them once, and you can define them on the fly in code too. Benefit number two: it makes the code more readable and descriptive, and less bug-prone.

Likewise, you already use name worksheets. You could use index numbers instead to refer to worksheets but obviously this is highly inflexible and not the best way to do it.

Besides, your file structure could evolve (adding, removing or moving some columns), which could introduce bugs if your references don't get shifted accordingly. And remember, they have to match all across your code. For example, Q4:AD appears on two lines. If you make it a named range, then you'll make the change at just one place. In fact you could declare all your named ranges at the beginning of the file so they are conveniently grouped together.

As shown by @PeterT, get into the habit of writing comments here and there, especially when the purpose of a block of code is not immediately clear, especially without knowing the data and the context. Everything is fresh in your mind today, but if you have to revisit your code in a few months to make changes or enhancements, you'll have forgotten why you did this and that and you'll have to reanalyze your code.

Comments also help delineate the various sections of your code and improve readability.

Misc:

  • get rid of unused variables (unnecessary clutter & possible confusion)
  • always assign a variable type, otherwise they are assumed variant (less performant type)
  • indentation & spacing is mostly ok, I sense an effort to keep it clean but at some places the End if is not on the same level as the starting If - again, indentation goes a long way toward making the code more clear and expressive

Unfortunately I have no idea what your code ultimately aims to accomplish. You have shown us the how but not the why. Perhaps there is a better way, perhaps Excel is the wrong tool for your need. I cannot even tell if those manipulations make sense. All I can do is provide tips to improve the current code. So this is merely a code review, not a technology assessment.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for all of your pointers. I will certainly start implementing some of these as i have indeed had to add more files. \$\endgroup\$
    – rmba22875
    Oct 3, 2022 at 13:45

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.