5
\$\begingroup\$

I am looking for best practice help. Slow loops seem to be a recurring problem for me and I'd like to learn a better way. The code itself works as it should, except it is far too slow.

The problem is the worksheet needs to calculate after each B & i value is dropped into "N13" so that "U12", "V12", and "W12" update before being deposited into wsRepository. If I turn Calculation on Manual then my values are no good because they are contingent upon the other worksheet formulas updating (calculating). I think I can copy my worksheets "off-screen", calculate, and then paste my values back "on-screen", but I am not sure how to do this. I've used variants in the paste to do similar things, but I not comfortable with them. There may even be more efficient ways of achieving my desired result that I am unaware of.

Application.ScreenUpdating = False

Dim wsRepository As Worksheet
Dim wsInput As Worksheet
Dim i As Integer

Set wsRepository = ThisWorkbook.Sheets("Repository")
Set wsInput = ThisWorkbook.Sheets("Input")

For i = 4 To 2004



           'add investment amount
           wsInput.Range("N13").Value = wsRepository.Range("B" & i).Value


           'copy back amounts
           wsRepository.Range("E" & i).Value = wsInput.Range("U12").Value
           wsRepository.Range("C" & i).Value = wsInput.Range("V12").Value
           wsRepository.Range("D" & i).Value = wsInput.Range("W12").Value


    Next i

wsInput.Activate
\$\endgroup\$
  • 1
    \$\begingroup\$ How complicated are the formulas in U12, etc.? Can you share them? It would be easier to help if you can. \$\endgroup\$ – RubberDuck Aug 17 '14 at 23:02
4
\$\begingroup\$
Application.ScreenUpdating = False

I find it scary that the corresponding = True is nowhere in your code, for reasons already mentioned. Whenever I turn off screen updating, I find it's good UX to also specify a status bar message, and change the mouse cursor to a hourglass. Something along these lines:

Public Sub ToggleWaitMode(Optional ByVal waitMode As Boolean = False)
    Application.ScreenUpdating = waitMode
    Application.Calculation = IIf(waitMode, xlCalculationManual, xlCalculationAutomatic)
    Application.StatusBar = IIf(waitMode, "Please wait...", vbNullString)
    Application.Cursor = IIf(waitMode, xlWait, xlDefault)
End Sub

Which makes your procedure stub look like this:

Option Explicit

Public Sub DoSomething()
    On Error GoTo ErrHandler
    ToggleWaitMode True

    'do that thing

CleanExit:
    If Not Application.ScreenUpdating Then ToggleWaitMode
    Exit Sub
ErrHandler:
    ToggleWaitMode
    ' handle errors here
    Resume CleanExit
End Sub

If I turn Calculation on Manual then my values are no good because...

If I understand properly, you need to update $N$13 some 2,000 times with a value that's in "$B$" & i, and then I guess $U$12, $V$12 and $W$12 need to be recalculated accordingly.

You haven't shown us what these cells contain and what cells their formula is referring to, but if they're the only cells that need to be recalculated when $N$13 changes, then you can force calculation like this:

wsInput.Range("$U$12").Calculate
wsInput.Range("$V$12").Calculate
wsInput.Range("$W$12").Calculate

But that might not speed up anything. You're pretty much stuck, since you need to recalculate these three cells before you can do anything, and you need to do that 2,000 times.

I think you're somewhat misusing VBA here, it looks like you could use 3 hidden columns (say, $AA$4:$AC$2004) and use Excel formulas to automatically calculate the would-be "U", "V" and "W" values for each row; the VBA macro could then just copy values from Input!$AA$4:$AC$2004 to Repository!$C$4:$E$2004.. if a macro is even needed for that.


I would suggest naming the ranges/cells in row 12 - anytime you have a specific cell with a specific meaning, it's always better for the VBA code to refer to the meaning rather than the cells' addresses.

I have no clue what these cells mean, but picture this:

Dim interestRate As Double
interestRate = wsInput.Range("InterestRate").Value

This extra abstraction level somewhat decouples the VBA code from the worksheet structure, which allows you to modify [at least parts of] the worksheet without having to modify the VBA code - for example you could insert another row and now InterestRate is read in row 13 instead of 12, and the VBA code couldn't care less.

You can define names in the [Formulas] Ribbon tab, under the [Defined Names] section. Or you can just select the cell and type its name in the address/names dropdown, just left of the formula bar.

This also has the advantage of making your Excel formulas more readable: instead of =$X$12*$N42 a formula can now look like =InterestRate*$N42


One last thing, I know it's common to call a worksheet variable like wsInput, but I find it sounds backwards and looks Hungarian. I'd call it inputSheet instead; wsRepository would be repositorySheet. Also it wouldn't hurt to rename i for row.

\$\endgroup\$
2
\$\begingroup\$

Two short things that even may be unneeded if yo u excluded them.

Use Option Explicit

For one it helps immensely when searching for errors with spelling. Been there done that. It's gruesome.
Secondly it helps you with writing code that's more similar to "real" programming languages. Nothing against vba, but I much prefer languages where you need to declare your variables.

Use an error handler.

Everytime you turn off screen updating you get into the dangerous zone of "breaking" the application in case somethin goes wrong.

Instead do something along the lines of:

Option Explicit
On Error Goto ErrorHandler

' a whole lot of code

:ErrorHandler
     Application.ScreenUpdating = true
     MsgBox "An error has occurred"
     'Whatever else you need to do and probably something like
     Exit Sub
\$\endgroup\$
2
\$\begingroup\$

I'm not sure this will work for you, but consider replacing or duplicating the formulas in your "12" cells with an Evaluate call. It's a little tricky to avoid runtime errors, so I suggest reading this. It might look something like this.

'wsRepository.Range("E" & i).Value = wsInput.Range("U12").Value
wsRepository.Range("E" & i).Value = Evaluate("SUM(A1:A10)")

Of course, using the formula in U12. Your mileage may vary, but this should let you set calculation to manual (I think). I would prefer the method Mat's Mug described though. This is just another option to try.

Some other notes

  • i is typically used as a loop counter, but row would be more meaningful.
  • 4 and 2004 are mysterious hardcoded numbers. What a lot of programmers refer to as magic numbers. It would good for readability/maintainability to replace them with startRow and lastRow constants.
  • Great comments in my opinion. They're short and clear. Not too much, not too little.
  • I'm not sure why you activate wsInput at the end, but you do a great job of avoiding it elsewhere, make sure you're not needlessly activating the sheet there.
\$\endgroup\$
  • \$\begingroup\$ I wouldn't expect Evaluate to speed things up though. \$\endgroup\$ – Mathieu Guindon Aug 18 '14 at 1:09
  • 2
    \$\begingroup\$ No. It wouldn't, but it may allow OP to use manual calculation @Mat'sMug and that would speed it up. (Assuming there are many other cells that wouldn't need to be recalculated). It's a stretch, but may be worth a shot. \$\endgroup\$ – RubberDuck Aug 18 '14 at 1:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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