Replacing a For Each loop with something faster

Question

I'm a very inexperienced VBA user, I've just created my first "For Each" loop but it's very slow. I'm happy that it works but eager to know if there is a alternative method that is quicker. Any advice would be appreciated but please bear in mind my inexperience!

My routine takes two variables "UserSelectionMin" and "UserSelectionMax" and uses them in conjunction with the RandBetween function to place a value in every cell in the selected range (all the selected cells will be empty).

Sub RandomScores()
Dim addr As String
addr = Selection.Address
' opens a userform and collects the min value, assigned to UserSelectionMin (which is declared as an integer)
MinScore
' opens a userform and collects the max value, assigned to UserSelectionMax (which is declared as an integer)
MaxScore
For Each c In Worksheets("matches").Range(addr).Cells
If Abs(c.Value) = 0 Then c.Value = WorksheetFunction.RandBetween(UserSelectionMin, UserSelectionMax)
Next
Unload RandomScoreFormMin
Unload RandomScoreFormMax
End Sub

The MinScore and MaxScore procedures are below although these bits seem to be working OK.

Sub MinScore()
With RandomScoreFormMin.ListBoxMin
.AddItem "0"
.AddItem "1"
End With
RandomScoreFormMin.Show vbModal
End Sub
Sub MaxScore()
With RandomScoreFormMax.ListBoxMax
 .AddItem "1"
 .AddItem "2"
 .AddItem "3"
End With
RandomScoreFormMax.Show vbModal
MsgBox "You selected " & UserSelectionMin & " as your min score and " & UserSelectionMax & " as your max score"
End Sub

• Vogel612: It looks a lot like you're missing a call to MaxScore before the For Each loop start.
• Mark Shepherd: Thanks Vogel612, I had missed that line out of the post. Although it does appear in my code. Have now added the missing line.
• Mark Shepherd: Thanks Mat's Mug - have now added code for MinScore and MaxScore.

Answer 1

This smells:

' opens a userform and collects the min value, assigned to UserSelectionMin (which is declared as an integer)
MinScore
' opens a userform and collects the max value, assigned to UserSelectionMax (which is declared as an integer)
MaxScore

You are using global variables - variables should have the tightest possible scope, and be passed to procedures as parameters; both MinScore and MaxScore should be Function procedures that return a value.

Sub MinScore()
With RandomScoreFormMin.ListBoxMin
.AddItem "0"
.AddItem "1"
End With
RandomScoreFormMin.Show vbModal
End Sub

Indentation is important for readability. It should look something like this:

Sub MinScore()
 With RandomScoreFormMin.ListBoxMin
 .AddItem "0"
 .AddItem "1"
 End With
 RandomScoreFormMin.Show vbModal
End Sub

However a more pressing issue is the fact that you are working against the form's default instance - which is a bad habit to take. Instead, you should create an object:

Dim view As RandomScoreFormMin
Set view = New RandomScoreFormMin
view.ListBoxMin.AddItem "0"
view.ListBoxMin.AddItem "1"
view.Show vbModal

and as I mentioned, that should be a function, which returns the user-selected min value:

Private Function GetMin() As Integer
 Dim view As RandomScoreFormMin
 Set view = New RandomScoreFormMin
 view.ListBoxMin.AddItem "0"
 view.ListBoxMin.AddItem "1"
 view.Show vbModal
 GetMin = view.ListBoxMin.Value
End Function

Now you no longer need to worry about loading/unloading the form, since a new instance is used every time this function is called. And then the calling code can use a local variable instead of a global:

Dim minValue As Integer
minValue = GetMin

---

Regarding the performance of the ForEach loop:

For Each c In Worksheets("matches").Range(addr).Cells
 If Abs(c.Value) = 0 Then 
 c.Value = WorksheetFunction.RandBetween(UserSelectionMin, UserSelectionMax)
 End If
Next

Taking the absolute value of the cell's content looks like a little hack - that would deserve a little comment to explain why it's done, otherwise a maintainer might deem it redundant, and remove it.

My guts are telling me that it's a way of getting an Integer value from the cell, even if the cell is empty. In that case, CInt(c.Value) would do a better job.

Another thing is that you are updating a worksheet in a tight loop; Excel is redrawing itself and recalculating the affected cells every time a value is written!

You can turn off screen updating and automatic calculation, while you're updating the sheet's contents:

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

Just don't forget to set ScreenUpdating back to True and calculation back to automatic after the loop completes (ideally in an error handler, so that if a runtime error occurs, screen updating is turned back on), so that Excel doesn't appear to remain "frozen"!

---

You have a potential bug here:

Dim addr As String
addr = Selection.Address

You're using the currently selected range's address...

For Each c In Worksheets("matches").Range(addr).Cells

...and then you're assuming that this range is on the "matches" worksheet - and it may not be all the time. If you mean to work with Selection, know that it is a Range, so you could do this instead:

For Each c In Selection.Cells

Otherwise, you need to validate the Selection before you use it - is it on the worksheet you're expecting?

---

Using WorksheetFunction is inherently slow, and should be avoided as much as possible; if your goal is to return a random number between X and Y, you could use VBA's random number generator instead of a worksheet function - here's a little formula to get a random integer between lowerbound and upperbound:

Int ((upperbound - lowerbound + 1) * Rnd + lowerbound)

Rnd returns a number that's smaller than 1, and greater than or equal to 0. It should be used in conjunction with the Randomize keyword:

Randomize
For Each c In Selection.Cells
 c.Value = Int((UserSelectionMax - UserSelectionMin + 1) * Rnd + UserSelectionMin
Next

• Mark Shepherd: Thank you Mat's Mug, that's incredibly helpful. I guess it might be a bit tiresome dealing with errors and anomalies from inexperienced users - I really appreciate your patience. I started at the bottom and used Rnd instead of RandBetween, then turned off the screenupdating, then turned off the autocalculation. Tested the code each time and it was the autocalculation that was slowing everything down. With that turned off, I can do 200 lines in about half a second - that's down from about 5 minutes so an enormous improvement. Now I'll work through your other suggestions. Thanks once again!
• Mathieu Guindon: That's what Code Review is for! Come back anytime with more working code! ;)
• Mathieu Guindon: @MarkShepherd as a side note, I'm working with another fellow VBA reviewer here, on a little project that might interest you - get the latest release of Rubberduck, a COM add-in for the VBA Editor, that can find code issues and help you refactor your VBA code (and lots, lots more) - we need people like you to find and report bugs in our little ducky! ;)

Answer 2

I'm a little unclear on the nature of some of the cells' values, i.e. whether they are typed values or cells containing formulas that may return either "" or 0. The Abs(c.Value) seems to indicate that you are looking to determine whether converted underlying values are equal to zero. This should get you started on a block processing routine that replaces the loop.

With Worksheets("matches").Range(addr).Cells
 .Replace What:=0, Replacement:=vbNullString, LookAt:=xlWhole
 With .SpecialCells(xlCellTypeBlanks)
 .Formula = "=RANDBETWEEN(" & UserSelectionMin & Chr(44) & UserSelectionMax & Chr(41)
 .Cells = .Value
 End With
End With

The worksheet formula is necessary as code like (Rnd * UserSelectionMax) + UserSelectionMin used on a block will not produce a random number for each cell; just one random number distributed to all of the cells.

• Mark Shepherd: Thanks Jeeped. All the cells in the selected range will be empty. Perhaps Abs(c.Value)=0 is incorrect? It did seem odd to me but worked so I left it alone!
• Jeeped: @MarkShepherd - Strictly speaking, zero is neither positive or negative so Abs(0) does nothing and neither -1 or 1 are equal to zero with or without the Abs(...). In short, it seems redundant.
• Jeeped: @MarkShepherd - If the cells to be considered will be blank and you are not trying to put a random number in a cell containing 0, then the .Replace(...) code line can be removed.