Calculating how long sales reps are idle

I wrote a procedure to calculate the number of months elapsed since a sales agent' last sale. It works but runs in over 10 minutes... for 20 rows.

Sub Latency()
' this sub-routine is used to calculate the no of recent idle months by an agent
Application.ScreenUpdating = False
For Each rng In Range("BL1:BL13000")
j = 1
Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag             
j = j + 1
If j > 12 Then Exit Do
Loop
ActiveCell.Value = (j - 1)   ' inserts the value to the cell
ActiveCell.Offset(1, 0).Select
Next rng
End Sub

First, let's give this some proper indentation so we can see what's going on.

Sub Latency()
    ' this sub-routine is used to calculate the no of recent idle months by an agent
    Application.ScreenUpdating = False
    For Each rng In Range("BL1:BL13000")
        j = 1
        Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag             
            j = j + 1
            If j > 12 Then Exit Do
        Loop
        ActiveCell.Value = (j - 1)   ' inserts the value to the cell
        ActiveCell.Offset(1, 0).Select
    Next rng
End Sub

Do you see it now? Freeman was absolutely right. You're not looping over 20 rows. You're looping over 13000. Why such a large range? Ohhh I know. You want to make sure you're getting all of the data, right? Okay, but what if the data range extends past row 13k? You don't just have a performance problem, you also have a correctness problem. Find the last row instead.

Let's see what else we've got while we're here though.

Sub Latency()

The default scope for VBA procedures is Public. So, you should ask yourself if this is the right thing here. I don't have enough context to say, but you should really be explicit whenever possible anyway. Don't rely on another developer's knowledge. Ever. Be explicit.

' this sub-routine is used to calculate the no of recent idle months by an agent

Cool! I like seeing short comments like this explaining what the purpose of the procedure is. It's a good comment. The problem is though, I can't figure out how the Sub's name (Latency) has anything to do with this. Try to come up with a better name for it.

Application.ScreenUpdating = False

Cool! Good idea! Make it look magical when you turn the screen updating back on! But wait.... you never turn it back on. That's bad. Real bad. If you're going to use this, you need to not only turn it back on at the end of the procedure, but you'll also need to make sure it always gets turned back on with an error handler.

For Each rng In Range("BL1:BL13000")

We talked about this already. Find the last row instead.

j = 1

Why are we starting with j? If you're going to use a generic counter variable, start at i and work your way up through j and k. It's just an expected convention.

What I'm actually worried about here is that you've not declared the variable. That means two things.

• You've not declared Option Explicit.

• This is implicitly a Variant type.

I cannot impress this on you enough. Use Option Explicit. It turns run time bugs into compile time bugs by forcing you to define all of your variables up front.

Why is Variant bad? It's bad because a variant can be anything. Yes. Anything. I could do this and VBA wouldn't blink twice.

j = 1
j = ActiveSheet

Seriously, yes. You can do this if you're using variant types. So don't.

Do Until ActiveCell.Offset(0, -23 - j) > 0    ' this reads the sales flag             
    j = j + 1
    If j > 12 Then Exit Do
Loop

• What are all these numbers??? I have no idea what this means at all. Use some well named constants instead.

• Great comment again! However, instead of using a comment, create a function that returns a boolean value.

• Avoid Activate and Select.

StackExchange Code Review Q#94111, answer score: 6