-1
\$\begingroup\$

I have found that my code causes my Excel to 'not respond' when it gets to this for loop, For thisRow = 2 To lastWSrow(outWS). I'm guessing this is because I am writing cell by cell and the data is too much for excel to handle.

My code essentially does 55 of vlookups on sheet2. It would be similar to doing vlookup(c2&y2,Sheet1!I:J,2,false) but manually it takes long to do.

Could anyone look at this loop and help me optimize this area? I have left only the part needed adjustment. I have put comments on each line to provide description.

outws = sheet2

For thisRow = 2 To lastWSrow(outWS) '2 to last row of sheet2
    For thisCol = 1 To UBound(mappings, 1)

        'create unique key
        thisScen = outWS.Cells(thisRow, posIDcol).Value & "|" & mappings(thisCol, 1)

        'search
        thisDataRow = findInArrCol(thisScen, 1, scenData)

        'write to sheet2
        If thisDataRow = 0 Then
            If outWS.Cells(thisRow, posUnitsCol).Value <> 0 Then 'missing scenario
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = "No data"
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = 0

            End If
        Else
            If mappings(thisCol, 1) = "irpv01|PV01_Swap_1M" Then
                outWS.Cells(thisRow, PV01_1MCol).Value = outWS.Cells(thisRow, PV01_1MCol).Value + scenData(thisDataRow, 2) * scenData(thisDataRow, 3) 'since map PV01_0D to PV01_1m already, sum PV01_1M to that column in database.
            Else
                outWS.Cells(thisRow, mappings(thisCol, 3)).Value = scenData(thisDataRow, 2) * scenData(thisDataRow, 3) 'need to scale by position units to get correct risk
            End If
        End If

    Next thisCol
Next thisRow

findInArrCol

Public Function findInArrCol(matchVal As Variant, matchCol As Long, sortedArr() As Variant, Optional nearest As String) As Long

    Dim low As Long, mid As Long, high As Long

    findInArrCol = 0
    low = 1
    high = UBound(sortedArr, 1)
    Do While low <= high
        mid = (low + high) / 2
        If sortedArr(mid, matchCol) = matchVal Then
            findInArrCol = mid
            Exit Function
        ElseIf sortedArr(mid, matchCol) < matchVal Then
            low = mid + 1
        Else
            high = mid - 1
        End If
    Loop

    If findInArrCol = 0 Then
        If nearest = "lessThan" Then
            findInArrCol = WorksheetFunction.Max(high, 1)
        End If
        If nearest = "greaterThan" Then
            findInArrCol = WorksheetFunction.Min(low, UBound(sortedArr, 1))
        End If
    End If

End Function
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, 200_success, t3chb0t, Billal Begueradj, Mast Jul 7 '18 at 19:31

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – t3chb0t, Billal Begueradj, Mast
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 7
    \$\begingroup\$ @Close-Voters: Excel going "Not Responding" isn't broken code, it's a single-threaded host application busy running VBA code, period. \$\endgroup\$ – Mathieu Guindon Jul 6 '18 at 16:33
  • 4
    \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does. \$\endgroup\$ – Phrancis Jul 6 '18 at 17:11
  • 5
    \$\begingroup\$ To reemphasize what @Phrancis said: you have a long history of posting questions that contain a long code dump along with a very vague description of what the code actually does. Please improve the quality of your titles and descriptions, following the site's How to Ask guidelines. \$\endgroup\$ – 200_success Jul 6 '18 at 17:47
  • 4
    \$\begingroup\$ Trimming the code down to an excerpt didn't really help. To be clear, questions that contain a lot of code can be acceptable on this site. But omitting information about what the expected inputs and outputs look like, and what the overall goal of the code is — that is not OK. See this revision history for an example of the improvements that it took to reopen a question. \$\endgroup\$ – 200_success Jul 6 '18 at 19:10
  • 2
    \$\begingroup\$ @TinMan Would you please write your answer into the answer box and not into a lot of different comments? Thanks :) \$\endgroup\$ – Vogel612 Jul 6 '18 at 21:34
4
\$\begingroup\$

As was explained in comments elsewhere, the Excel VBA thread is sending messages to the WM message queue to process information, but the same thread is running the for/next loop(s) and will not begin as you had the code until the loops are complete. By inserting a DoEvents, it causes the VBA code to perform slower because it then allocates some time to the O/S to take care of other pending WM messages and returns back to the VBA code. Too many DoEvents will greatly slow down your code processing and can even be harmful in some cases where another process can actually take over (hog) more processor time. The solution is to use DoEvents sparingly to allow Excel to process the rows a few at a time. What the ideal number is varies by situation, but you can experiment with various batch sizes determined by using modulo arithmetic.

Modify your loop in this way:

        Next thisCol
        If (thisRow Mod 100) = 0 Then
            DoEvents
        End If
    Next thisRow

You can tweak this number by replacing 100 with 10, 50, 250, 500, etc. to see what provides the most desirable result.

\$\endgroup\$
  • \$\begingroup\$ While I've been programming in BASIC my entire career, my exposure to Excel or any other Office apps (VBA) in general is limited, so I'm not going to be able to expound much more than I have already as to code review. At least the code indentations make it readable, amazing how many code examples I see with little, no, or improper indentation. I don't expect any upvotes, just wanted to offer to help with DoEvents, really. \$\endgroup\$ – Bill Hileman Jul 6 '18 at 19:47
0
\$\begingroup\$

A couple of points:

At one stage, you write to the same cell twice. I don't know how many times you loop, but this will certainly add to the time:

                    outWS.Cells(thisRow, mappings(thisCol, 3)).Value = "No data"
                    outWS.Cells(thisRow, mappings(thisCol, 3)).Value = 0

We can't see how you findInArrCol(thisScen, 1, scenData). For all we know, this could be an expensive and inefficient operation. I would guess that this is looping within a loop (that is also within a loop) - O(n2) at least? Especially if your "find" is looping through a range instead of an array.

Without enough context, cannot provide any further help than that.

\$\endgroup\$
  • \$\begingroup\$ thanks AJD, I added the findInArrCol function to the question. \$\endgroup\$ – excelguy Jul 7 '18 at 22:18
-1
\$\begingroup\$
Application.Calculation = xlCalculationManual
Application.ScreenUpdating = False

' Place actual code between the above and below

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True    

The above will generally fix what you are experiencing.

The first part turns off the auto calculation that happens after any change to the sheet. It also turns off screen updating, so it isn't showing you the changes that are taking place.

Last part turns both back on, so the document behaves as normal.

\$\endgroup\$
  • 4
    \$\begingroup\$ The above will generally fix what you are experiencing - no, it won't. If anything it'll just make Excel go "not responding" faster. DoEvents, i.e. allowing Excel to process messages piling up in its WM message pump, will fix it... but will also make the VBA code complete slower. VBA runs on the host application's main/UI thread, so if that thread is busy running VBA code, it can't respond to its message pump simultaneously. That said this answer doesn't review the PO's code, either. \$\endgroup\$ – Mathieu Guindon Jul 6 '18 at 17:57
  • \$\begingroup\$ Thanks for your reply Jack, I have tried your suggestion and still getting the answer. Mathieu was correct with his comment. \$\endgroup\$ – excelguy Jul 6 '18 at 18:30
  • 1
    \$\begingroup\$ actually, this can be the default answer to nearly all excel-is-slow questions ;-) \$\endgroup\$ – t3chb0t Jul 6 '18 at 18:53
  • 2
    \$\begingroup\$ @t3chb0t and in most cases it would continue to be something between wrong and useless... ScreenUpdating and Calculation is but one factor in slowdown for excel. Much more speed can be gained by avoiding access to Range objects, by using With, by moving accessors to outside of loops and generally all other speed-improvements \$\endgroup\$ – Vogel612 Jul 6 '18 at 19:07

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