1
\$\begingroup\$

My VBA code currently loops for 5,000 iterations and takes about 25 minutes to collect the output data in the Output excel sheet. Is there a way to reduce the time taken for instance by improving the copying and pasting of values, collecting the output data in some kind of array etc.? Given that I will be looping it for up to 10,000 iterations when I obtain more data

Note: The calculations are excel formulas within the Calcs excel sheet, as this is a cash flow schedule with irregular cash flows and discount rates, hence I am unable to automate the cash flow calculations via VBA.

Sub x()

Dim r As Long
Dim lastrow As Long

Application.ScreenUpdating = False
Application.CutCopyMode = False

With Worksheets("MCInput")
lastrow = .Cells(.Rows.Count, "B").End(xlUp).Row
MsgBox lastrow - 1 & " Trials"
End With

With Worksheets("Calcs")
    For r = 0 To (lastrow - 2)
        .Range("CASHINPUT").Value = Worksheets("MCInput").Range("TCASHRTN").Offset(r).Value
        .Range("EQINPUT").Value = Worksheets("MCInput").Range("TEQRTN").Offset(r).Value
        .Range("FIINPUT").Value = Worksheets("MCInput").Range("TFIRTN").Offset(r).Value
        .Range("Results").Calculate
        .Range("Results").Copy
        Worksheets("Output").Range("A1").Offset(1 + r, 1).PasteSpecial xlPasteValues
    Next r
End With

Application.ScreenUpdating = True
Application.CutCopyMode = True

End Sub

\$\endgroup\$
7
  • 1
    \$\begingroup\$ You mention this is a cash flow schedule, can you add a little more detail about that? \$\endgroup\$
    – pacmaninbw
    Dec 8, 2019 at 14:31
  • 1
    \$\begingroup\$ You've an End Sub but no (Public | Private) Sub FooBar() to start it. I'm assuming it goes above your variable declarations, but that could be incorrect. \$\endgroup\$
    – IvenBach
    Dec 8, 2019 at 18:14
  • \$\begingroup\$ What's the Results range calculating and why does it need to be calculated by Excel, as opposed to being computed by VBA code? If that calculation doesn't need to happen on the worksheet, then you can collect the inputs, perform all calculations, and dump all results at once, with 1 single worksheet read and 1 single worksheet write. \$\endgroup\$ Dec 9, 2019 at 0:04
  • 2
    \$\begingroup\$ Is it really called x though? You see naming is one of the hardest things in programming: giving a meaningful name to a procedure that does too many things can even be impossible! Typically, procedure names start with a verb, and describe the purpose or side effects of the procedure... anyway it would be nice to have the formula for the Results range, and enough context to be able to determine whether a formula whose result is copied and then pasted, is the most efficient way to go about it. \$\endgroup\$ Dec 9, 2019 at 4:36
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Dec 9, 2019 at 8:51

2 Answers 2

3
\$\begingroup\$

Use descriptive variable names. r as a variable name tells you nothing and makes your code harder to read. rowOffset tells you what it's used for. This goes a long way to make your code readable.

Declare your variables just before you use them. You only have 2 currently but its easy for variables to increase. Instead of having a wall of declarations at the top of a Function/Sub, like below:

dim r as long
Dim i as long
dim p as range
dim sv as range

Have them declared just before they get used.

dim lastRow as long
lastRow = Worksheets("MCInput").Cells(Rows.Count,"B").End(xlUp).Row

dim rowOffset as long
For rowOffset = 0 to (lastRow - 2)
    `Code that does what you want
Next

This ultimately aids you in refactoring (restructuring) your code when you need to. It also becomes easier to tell when a variable is no longer needed. No code using the variable just after it's declared? Delete it. With a wall of declarations you have to laboriously check for usage. There are tools like Rubberduck, which I'm a contributor to, than can tell you if a variable is used.


You're using a string literal populated with a worksheets name to access the worksheet through the hidden Global object's Worksheets property IE Global.Worksheets("Calcs"). This returns an Object variable which is late-bound and doesn't give you intellisense when you use . (period) after the collection. It's preferred to use the Worksheet.CodeName. You can see this in the Project Explorer window. The CodeName of the worksheet is Sheet1 while the Name (What is shown in the tab) is Calcs.

enter image description here

Sheet1 isn't a helpful name, this ties into using descriptive names above, so we'll change it by double clicking on it in the Project Explorer to display the code behind for the worksheet, then pressing F4 to display the Properties Window (also found under the menu at the top View>Properties. At the top where it says (Name) change it to something more descriptive.

enter image description here

Now instead of using Worksheets("Calcs") use CalculationSheet to access the worksheet object. The same thing can be done to. Two added benefits:

  • You're no longer dependent on the literal string "Calcs" which stops working if the worksheet is renamed.
  • You get intellisense. Type CalculationSheet. (note the period) to show a list of accessible members.

There's more that can and probably should be done with your code. Things like worksheet properties would help to clean up and shorten the code which then leads to the realization you'd benefit from creating a proxy for accessing your data. I was nerd-sniped by this already and that can be for another review.

\$\endgroup\$
0
\$\begingroup\$

Thanks, guys. I got the answer on another forum. I made it store the calculated data outputs in an array before performing a one-time dump into another excel sheet. This halved my time taken to run the loops.

\$\endgroup\$

Your Answer

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

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