1
\$\begingroup\$

I've got an issue where my looping through the rows on my sheet are extremely slow. For 20,000 rows, it takes about 20 minutes, whereas before, it was taking 3. The only pieces I added were at the very bottom, starting with the

for l = 2 to lastLet

and ending at the bottom of the code.

When I go to debug the code while running to see where it's at, I am surprised to find that the hangup isn't at these last lines, but rather that it sits in the middle of the code. Unfortunately it's for billing data so I can't share the sheet, but the data comes to me in a simple database format.To note, every piece of the code still works (and yes I know initializing variables is good practice), but is there a way to increase the performance on it?

Is the addition I made to the code causing the slowing in run time or is it something else?

Note: I don't have any formal education with programming or VBA. All information is self taught, so please use terminology I can understand~

Function Col_Letter(colInt As Integer) As String
' This function converts column numbers to their corresponding letters
Dim lett
lett = Split(Cells(1, colInt).Address(True, False), "$")
Col_Letter = lett(0)
End Function

`

Sub OpenBilling()
Dim lastLet As Integer
Sheets("Billing").Select
lastRow = Sheets("Billing").range("A" & Rows.Count).End(xlUp).Row
lastLet = ActiveSheet.Cells(1, Columns.Count).End(xlToLeft).Column
lastCol = Col_Letter(lastLet)
sort1 = Col_Letter(Sheets("Billing").range("1:1").Find("ccustname").Column)
sort2 = Col_Letter(Sheets("Billing").range("1:1").Find("cjobno").Column)
sort3 = Col_Letter(Sheets("Billing").range("1:1").Find("dinvdate").Column)
sort4 = Col_Letter(Sheets("Billing").range("1:1").Find("citemdesc").Column)
sort5 = Col_Letter(Sheets("Billing").range("1:1").Find("csort3").Column)
sort6 = Col_Letter(Sheets("Billing").range("1:1").Find("ninvamt").Column)
ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Clear
ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Add Key:=range( _
sort1 & "2:" & sort1 & lastRow), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
xlSortNormal
ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Add Key:=range( _
sort2 & "2:" & sort2 & lastRow), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
xlSortTextAsNumbers
ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Add Key:=range( _
sort3 & "2:" & sort3 & lastRow), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
xlSortNormal
ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Add Key:=range( _
sort4 & "2:" & sort4 & lastRow), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
xlSortNormal
With ActiveWorkbook.Worksheets("Billing").Sort
.SetRange range("A1:" & lastCol & lastRow)
.Header = xlYes
.MatchCase = False
.Orientation = xlTopToBottom
.SortMethod = xlPinYin
.Apply
End With
Sheets("Billing").range("A:A").Insert (xlShiftToRight)
sort1 = Col_Letter(Sheets("Billing").range("1:1").Find("ccustname").Column)
sort2 = Col_Letter(Sheets("Billing").range("1:1").Find("cjobno").Column)
sort3 = Col_Letter(Sheets("Billing").range("1:1").Find("dinvdate").Column)
sort4 = Col_Letter(Sheets("Billing").range("1:1").Find("citemdesc").Column)
sort5 = Col_Letter(Sheets("Billing").range("1:1").Find("csort3").Column)
sort6 = Col_Letter(Sheets("Billing").range("1:1").Find("ninvamt").Column)
change1 = Sheets("Billing").range("1:1").Find("ninvamt").Address
Sheets("Billing").range("A1").Value = "Difference"
Sheets("Billing").range("A2").Value = "=IF(H2=H1,IF(OR((" & sort3 & "2-" & sort3 & "1)<(-1000),ISERROR(" & sort3 & "2-" & sort3 & "1)),""""," & sort3 & "2-" & sort3 & "1),0)"
Sheets("Billing").range("A2").Copy
Sheets("Billing").range("A2:A" & lastRow).PasteSpecial
Sheets("Billing").range("A1").Select
Selection.Subtotal GroupBy:=Sheets("Billing").range("1:1").Find("cjobno").Column, Function:=xlSum, _
TotalList:=Array(Sheets("Billing").range("1:1").Find("ninvamt").Column), _
Replace:=True, PageBreaks:=False, SummaryBelowData:=True

lastRow = Sheets("Billing").range("A" & Rows.Count).End(xlUp).Row
lastLet = ActiveSheet.Cells(1, Columns.Count).End(xlToLeft).Column
Columns(sort3 & ":" & sort3).Select
Selection.Replace What:="  -   -", Replacement:="0", LookAt:=xlPart, _
SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
ReplaceFormat:=False
For i = 1 To lastRow
If Sheets("Billing").range(sort1 & i).Value = "" Then
Sheets("Billing").range(sort1 & i).Value = "=" & _
Sheets("Billing").range(sort1 & (i - 1)).Address
End If
If Sheets("Billing").range(sort5 & i).Value = "" Then
Sheets("Billing").range(sort5 & i).Value = "=" & _
Sheets("Billing").range(sort5 & (i - 1)).Address
End If
If Sheets("Billing").range("B" & i).Value = "" Then
endFor = Sheets("Billing").range(sort3 & (i - 1)).Address
If startFor = "" Then
startFor = "$" & sort3 & "$2"
Else: Sheets("Billing").range(sort3 & i).Value = "No Date"
End If
Sheets("Billing").range(sort3 & i).Value = "=Max(" & _
startFor & ":" & endFor & ")"
End If
If Sheets("Billing").range("B" & i).Value = "" Then
endFor = Sheets("Billing").range(sort3 & (i - 1)).Address
If startFor = "" Then
startFor = "$" & sort3 & "$2"
End If
startNum = range(startFor).Row
endNum = range(endFor).Row
maxRange = 0
currMax = 0
For k = endNum To startNum Step -1
If Sheets("Billing").range(sort3 & k).Value <> "" Or _
Sheets("Billing").range(sort3 & k).Value <> 0 Then
If maxRange < Sheets("Billing").range(sort3 & k).Value Then
currMax = Sheets("Billing").range(sort3 & k).Offset(0, 9).Value
maxRange = Sheets("Billing").range(sort3 & k).Value
End If
Else
End If
Next k
Sheets("Billing").range(sort3 & i).Offset(0, 9).Value = currMax
End If
If Sheets("Billing").range("B" & i).Value = "" Then
Sheets("Billing").range("B" & i).EntireRow.Font.Bold = True
End If
If Sheets("Billing").range("B" & i).Value = "" Then
endFor = Sheets("Billing").range(sort3 & (i - 1)).Address
If startFor = "" Then
startFor = "$" & sort3 & "$2"
End If
startNum = range(startFor).Row
endNum = range(endFor).Row
maxRange = 0
For j = endNum To startNum Step -1
If Sheets("Billing").range(sort3 & j).Value <> "" Or _
Sheets("Billing").range(sort3 & k).Value <> 0 Then
If maxRange < Sheets("Billing").range(sort3 & j).Value Then
currMax = Sheets("Billing").range(sort3 & j).Offset(0, 1).Value
maxRange = Sheets("Billing").range(sort3 & j).Value
End If
End If
Next j
Sheets("Billing").range(sort3 & i).Offset(0, 1).Value = currMax
startFor = Sheets("Billing").range(sort3 & (i + 1)).Address
End If
Next i
range(sort1 & "1").Value = "Builder"
range(sort5 & "1").Value = "Project Name"
range(sort2 & "1").Value = "Job Number"
range(sort3 & "1").Value = "Invoice Date"
range(sort4 & "1").Value = "Item Description"
Sheets("Billing").range(change1).Value = "Invoice Amount"
Columns.AutoFit
For l = 2 To lastLet
If Sheets("Billing").range(Col_Letter(CInt(l)) & "1").End(xlDown).Row < lastRow Then
Sheets("Billing").range(Col_Letter(CInt(l)) & ":" & Col_Letter(CInt(l))).ClearContents
End If
Next l
Dim delRange As range
Set delRange = ActiveSheet.UsedRange
For counter = delRange.Columns.Count To 1 Step -1
If Application.CountA(Columns(counter).EntireColumn) = 0 Then
Columns(counter).Delete (xlShiftToRight)
End If
Next counter
MsgBox "The report has been completed."
End Sub
\$\endgroup\$
  • 2
    \$\begingroup\$ Declaring your variables will already remove the need for VBA to declare them on-the-fly as implicit Variant, which will in turn remove the need to perform on-the-fly implicit type conversions everywhere they're used, which should already help performance at least a tiny little bit. There's a reason why declaring all your variables is a good practice. If you know about it, then why aren't you doing it? Specify Option Explicit to prevent VBA from happily compiling typos and introducing hard-to-find and oh-so-frustrating bugs. \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 15:55
  • \$\begingroup\$ I would warmly recommend using Rubberduck (a VBIDE add-in OSS project I manage) to help you with locating all undeclared variables and properly indent your code with one single click, enhancing readability ten-fold, pretty much effortlessly. \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 15:58
  • \$\begingroup\$ Can you please edit your post so that it includes at least everything from Sub ... to End Sub? \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 15:59
  • \$\begingroup\$ @Mat'sMug Is it really that big of a performance difference using declared versus undeclared variables? Also, what's there is everything from Sub to End Sub, there's nothing special on those sides. As for your IDE, I usually manually indent my code, but had some......issues with my management that caused me to not have the time to change it, then left over a weekend. I appreciate the help and the IDE link! \$\endgroup\$ – Anoplexian - Reinstate Monica Sep 27 '17 at 16:06
  • 1
    \$\begingroup\$ Experience talking: you probably won't notice. Your users probably won't notice. It's not about performance, it's about correctness and avoiding stupid typo-induced bugs that take an hour to find. Option Explicit is your best friend here, trust me. I'm sure you'll love Rubberduck BTW! \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 16:09
4
\$\begingroup\$

First, kudos and welcome to the wonderful world of programming! I'm self-taught as well! One thing I've learned - and that's not specific to programming in any way - is that dumbing-down explanations serves no one: if there's anything in my post that isn't clear, feel free to ask. Also feel free to google up any terms you're possibly not familiar with.

In other words I'll try to keep it accessible, but there won't be any flowers & bees here.

The first thing that strikes anyone looking at this code, is the sheer lack of breathing space, both vertical and horizontal: everything is crammed into a monolithic unindented block of procedural code that does everything that needs to happen: that procedure is a "God procedure", a script that essentially goes top-to-bottom sequentially, and that's all there is to it.

Whenever you see a chunk of code and think "that chunk of code does X" or "these 10 lines here are responsible for Y", the single best thing to do is to add a comment that says what it does extract it into its own, separate, dedicated procedure. Doing that will increase the abstraction level of your "main" procedure, making it progressively less and less concerned about every single little detail of what's going on, and more and more about the big picture: once you're done splitting it up, you should be able to read that "main" procedure as a series of operations, and at a glance know what it does, without necessarily knowing how it does it.


You never need to work out the "letter" of a column. Never. Kill Col_Letter with fire, and don't look back.

When you know the column index you need to work with, instead of concatenating it with a row number to use it with Range, use it directly with Cells (which also happens to return a Range object), which uses row and column numbers.

Side note, avoid underscores in procedure names. The underscore has a special meaning in VBA code, and when you get to a more advanced level they can bite you in the rear end and literally prevent your code from compiling. Stick to PascalCase and everything will be fine.


I did a quick search on this page for Sheets("Billing"), and my browser finds 62 instances.

That means you're dereferencing the same object 62 times. And you're not consistent with how you're doing it; sometimes it's through the Sheets collection, implicitly off the ActiveWorkbook:

sort6 = Col_Letter(Sheets("Billing").range("1:1").Find("ninvamt").Column)

Other times it's through the Worksheets collection, explicitly off the ActiveWorkbook:

ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Clear

Interesting that these two lines are one after the other.

Other times you're just working off the ActiveSheet explicitly - but it's the same object reference again, since the sheet was .Selected:

lastLet = ActiveSheet.Cells(1, Columns.Count).End(xlToLeft).Column

If that Billing sheet is part of ThisWorkbook (the workbook that contains this code), then give it a code name - select the worksheet in the VBE's Project Explorer, and hit F4 to bring up the Properties toolwindow, and change its (Name) property to, say, BillingSheet. Note that if anything in your code is referencing the sheet's current CodeName, renaming it that way will break your code.

Or, right-click it in Rubberduck's Code Explorer, and select "Rename" - Rubberduck will change its CodeName property for you, and if anything in your code is using that CodeName identifier, it will also be updated.

Then, use it. Instead of this:

ActiveWorkbook.Worksheets("Billing").Sort.SortFields.Clear

Do this:

BillingSheet.Sort.SortFields.Clear

How does that work?

By setting the (Name) property of the worksheet to BillingSheet, VBA creates a globally-scoped object variable named after the value of that property. By default that's e.g. Sheet1, but you can change that and use this free global object variable everywhere, instead of dereferencing that very same object every time from a collection.


Next thing to do is to remove all .Select calls and anything that's working with Selection:

Sheets("Billing").range("A1").Select
Selection.Subtotal GroupBy:=Sheets("Billing").range("1:1").Find("cjobno").Column, Function:=xlSum, _
TotalList:=Array(Sheets("Billing").range("1:1").Find("ninvamt").Column), _
Replace:=True, PageBreaks:=False, SummaryBelowData:=True

Instead, work with the Worksheet and Range objects you have:

BillingSheet.Range("A1").Subtotal _
    GroupBy:=BillingSheet.Range("1:1").Find("cjobno").Column, _
    Function:=xlSum, _
    TotalList:=Array(BillingSheet.Range("1:1").Find("ninvamt").Column), _
    Replace:=True, _
    PageBreaks:=False, _
    SummaryBelowData:=True

Notice how much easier that is to read!

For i = 1 To lastRow
If Sheets("Billing").range(sort1 & i).Value = "" Then
Sheets("Billing").range(sort1 & i).Value = "=" & _
Sheets("Billing").range(sort1 & (i - 1)).Address
End If

Indentation is your friend too:

For i = 1 To lastRow
    If BillingSheet.Cells(i, customerNameColumn).Value = "" Then
        BillingSheet.Range(i, customerNameColumn).Value = _
            "=" & BillingSheet.Range(i - 1, customerNameColumn).Address
    End If
    '...

In your code, I have no idea where that loop block ends, and I don't have the patience to look for it.

Undeclared variables are a plague, too: unless I quadruple-check everything I do (who has time for that?), I can't be sure I won't be introducing a bug by moving a startFor assignment somewhere else, or just rearranging the code.

This isn't about performance, it's about maintainability. Code like this is unmaintainable. Over time, the only thing that can possibly happen, is that it rots and starts collecting bugs, and as old bugs are fixed, new subtle ones inevitably get introduced.

There's one right here:

    If BillingSheet.Cells(i, customerNameColumn).Value = "" Then

If BillingSheet.Cells(i, customerNameColumn).Value contains an Excel error value (e.g. #N/A or #VALUE!, or whatever), then that conditional will be blowing up with run-time error 13 / Type Mismatch, because you can't legally compare an Error to a String - or anything actually. Any time you read a cell value, you should wrap it with an IsError function call first, to make sure the call won't blow up on you because some user deleted a column, or something on the worksheet is dividing by zero, or a lookup failed somewhere, or whatever.

That's as far as I'll go.

You can definitely improve the performance of that code, but doing that without first addressing the readability/maintainability issues would be dangerous.

You can use Union to combine the entire rows you want to delete, and delete them all at once, instead of looping backwards and recalculating the entire worksheet every time.

Oh, well there's an easy way to improve performance - prevent Excel from constantly recalculating everything every time you change a cell, and heck, prevent it from repainting and firing worksheet events every time, too:

Application.Calculation = xlCalculationManual
Application.EnableEvents = False
Application.ScreenUpdating = False

' do stuff

Application.Calculation = xlCalculationAutomatic ' triggers recalc
Application.EnableEvents = True
Application.ScreenUpdating = True

That should help quite a lot, but you definitely want to break up that procedure into smaller chunks, declare every variable, indent everything properly, and stop querying worksheet collections all the time.

I hope to see a new question soon, with the revised code!

\$\endgroup\$

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.