Improve the performance of a if else condition inside a loop

Question

I wrote a VBA macro & I want to improve the performance because the macro takes ages to run.

I think that the running performance is impacted by the

For Each rCell In .Range("O3:O" & Range("O" & Rows.Count).End(xlUp).Row) which intend to limit the loop up to the first empty row.

Sub E_Product_Density_Check()

Dim ws As Worksheet

Set Vws = ThisWorkbook.Sheets("Variables")

Sheets("Sheet1").Select

Application.ScreenUpdating = False
For Each ws In ActiveWorkbook.Worksheets
If ws.Name <> "Variables" Then

 Application.DecimalSeparator = ","

ws.Activate

With ActiveSheet
        For Each rCell In .Range("O3:O" & Range("O" & Rows.Count).End(xlUp).Row)
        For Each iCell In .Range("N3:N" & Range("N" & Rows.Count).End(xlUp).Row)
        For Each xCell In .Range("M3:M" & Range("M" & Rows.Count).End(xlUp).Row)
        For Each yCell In .Range("L3:L" & Range("L" & Rows.Count).End(xlUp).Row)

            If (rCell.Value / ((iCell.Value * xCell.Value * yCell.Value) / 1000000)) <= Application.WorksheetFunction.VLookup(ActiveSheet.Name, Vws.Range("A1:E10"), 5, False) Then
                rCell.Interior.Color = vbYellow
            Else
                rCell.Interior.Color = vbWhite
            End If
        Next yCell
        Next xCell
        Next iCell
        Next rCell
    End With
    End If
    Next ws
End Sub

It is hard to see what the code is doing, but nested for-loops nested to a depth of 4 is quartic complexity. n^4 gets very large very fast. In any event, if the purpose is to color cells -- why not just use conditional formatting? — John Coleman
– John Coleman, Commented Feb 18, 2019 at 15:19
First, set somevalue = Application.WorksheetFunction.VLookup(ActiveSheet.Name, Vws.Range("A1:E10"), 5, False) outside of the loop, and use it in your comparison, so you don't have to actually do the same vlookup on every loop — drgs
– drgs, Commented Feb 18, 2019 at 15:20
@JohnColeman - it is actually n^5, worksheets are also looped. — Vityata
– Vityata, Commented Feb 18, 2019 at 15:20
@Comintern And it seems that OP could probably use such a review. For example, the constant accessing of individual cell values would need to be addressed. — John Coleman
– John Coleman, Commented Feb 18, 2019 at 15:24
I think you don't understand what nested loops are for. The only thing you change is rCell's color. But why would you change it n^4 time if you keep only the last calculation for each set of cells? Ox's color will be determined based on Ox, Nn, Mn, and Ln, so n iterations should be enough. — z32a7ul
– z32a7ul, Commented Feb 18, 2019 at 15:39

John Alexiou · Accepted Answer · 2019-02-18 17:23:28Z

1

Try this:

Sub E_Product_Density_Check2()
    Dim ws As Worksheet, Vws As Worksheet
    Set Vws = ThisWorkbook.Sheets("Variables")

    Sheets("Sheet1").Select
    ' Application.ScreenUpdating = False  (no need for this)
    Application.DecimalSeparator = ","

    Dim target As Variant
    Dim r_O As Range, r_N As Range, r_M As Range, r_L As Range
    Dim n As Long
    Dim i As Long

    For Each ws In ActiveWorkbook.Worksheets
        If ws.Name <> "Variables" Then
            ' For the target value for each worksheet
            target = Application.WorksheetFunction.VLookup(ws.Name, Vws.Range("A1:E10"), 5, False)
            ' ws.Activate  (this was slow)

            'Find the number of cells in column O, and assume the same number exists in N, M & L.
            n = ws.Range(ws.Range("O3"), ws.Range("O3").End(xlDown)).Rows.Count
            Set r_O = ws.Range("O3")
            Set r_N = ws.Range("N3")
            Set r_M = ws.Range("M3")
            Set r_L = ws.Range("L3")

            For i = 1 To n
            ' Go down the column O
                If (r_O.Cells(i, 1).Value / ((r_N.Cells(i, 1).Value * r_M.Cells(i, 1).Value * r_L.Cells(i, 1).Value) / 1000000)) < target Then
                    r_O.Cells(i, 1).Interior.Color = vbYellow
                Else
                    r_O.Cells(i, 1).Interior.Color = vbWhite
                End If
            Next i
        End If
    Next ws
End Sub

I think what you are trying to do is set the color of column O based on the values of columns M, N & L in the same row.

The reason I came to this conclusion is because with your code the color of column O cell is determined only by the values in the last rows only since each iteration of the inner loops overwrites the same cell.

edited Feb 18, 2019 at 17:23

answered Feb 18, 2019 at 17:15

John Alexiou

29.4k12 gold badges83 silver badges136 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

brice Over a year ago

Hi @ja72, your code works perfectly and is much more performant. Thank you. I really appriciate.

John Alexiou Over a year ago

Lessons learned. 1) do not use ActiveSheet and .Activate (or blind Range() which implies ActiveSheet). 2) Use the .Cells() propery to pull a specific cell from a table using numeric values for the row and column index. 3) Do not use string math as in "O3:O" & Range("O" & Rows.Count).End(xlUp).Row.

John Coleman Over a year ago

@brice if this code answers your question to your satisfaction, you should mark it "accepted" (by selecting the check mark next to the question)

Cyril · Accepted Answer · 2019-02-18 17:03:03Z

0

Is this what you're trying to do? Snippit:

    Dim r as long, lr as long, myvalue as double 'r is row to iterate, lr is last row, myvalue = your vlookup
    'skipping the other code to get down to the loop
    With ActiveSheet
        myvalue = Application.WorksheetFunction.VLookup(ActiveSheet.Name, Vws.Range("A1:E10"), 5, False) 'shoudl only need to find this once
        lr = .cells(.rows.count,"O").end(xlup).row
        For r = 2 to lr 'starting on 2 because 1 is probably headers
            If (.Cells(r,"O").Value / ((.Cells(r,"N").Value * .Cells(r,"M").Value * .Cells(r,"L").Value) / 1000000)) <= myvalue Then
                .Cells(r,"O").Interior.Color = vbYellow
            Else
                .Cells(r,"O").Interior.Color = vbWhite
            End If
        Next r
    End With

answered Feb 18, 2019 at 17:03

Cyril

6,8291 gold badge21 silver badges33 bronze badges

1 Comment

brice Over a year ago

Works great too. Thanks a lot

Collectives™ on Stack Overflow

Improve the performance of a if else condition inside a loop

2 Answers 2

3 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related