2
\$\begingroup\$

I'm working on a large VBA project for work (around 1000+ lines of vba code) This module is called within another module where I disable everything possible like screen updating, events and setting calculation to manual to improve speed, yet this still seems to run slowly when I'm working with sheets with over 60,000 rows.

What this code does, is filter a row in one sheet and copy paste it into another sheet. Then use the data from this to vlookup onto another sheet. I don't understand the syntax of index match, otherwise I could have eliminated the cut/paste option to move the column on the right for the vlookup to speed it up.

I suspect a few other elements in this code chunk are slowing it down too. Could you please help me identify what is causing the issue? Is it the autofilter?

Sub AdgroupName()

Dim LrowBlk As Long
LrowBlk = Sheets("Bulk Sheet").Cells(Rows.Count, "H").End(xlUp).Row
Sheets("Bulk Sheet").AutoFilterMode = False
Sheets("Bulk Sheet").Range("H3:I3").AutoFilter field:=2, Criteria1:="AdGroup"
ActiveWorkbook.Sheets("Bulk Sheet").Range("H3:K3" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy
Worksheets("Adgroup List").Paste Destination:=Worksheets("Adgroup List").Range("A1")
ActiveWorkbook.Sheets("Bulk Sheet").Range("P3:P" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy
Worksheets("Adgroup List").Paste Destination:=Worksheets("Adgroup List").Range("E1")
With Sheets("Adgroup List")
    .Columns("C:C").Cut
    .Columns("E:E").Insert Shift:=xlToRight
 End With
Application.CutCopyMode = False
Dim ws As Worksheet
Dim LrowSTR As Long
Set ws = ThisWorkbook.Sheets("Search Term Report")
LrowSTR = Sheets("Search Term Report").Cells(Rows.Count, "H").End(xlUp).Row
Dim Strformulas(1 To 3) As Variant

With ws
    Strformulas(1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
    Strformulas(2) = "=VLOOKUP($K4,'Adgroup List'!$C:$E,3,0)"
    .Range("AJ4:AK4").Formula = Strformulas
    .Range("AJ4:AK" & LrowSTR).FillDown
    .Range("AJ3").Value = "Campaign ID"
    .Range("AK3").Value = "AdGroup Name"
    End With
     Sheets("Bulk Sheet").AutoFilterMode = False
     Sheets("Adgroup List").AutoFilterMode = False
     End Sub
\$\endgroup\$
  • 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\$ – Toby Speight Jun 22 '18 at 12:57
  • 1
    \$\begingroup\$ The new title still states the concern about your code and, as a consequence, is still too generic. It should reflect what your code does. The concern should be part of the actual question only. \$\endgroup\$ – M.Doerner Jun 22 '18 at 13:24
  • 1
    \$\begingroup\$ Rule of thumb: "this code" in a title is a bit of a red flag. Tell us what "this code" is/does =) \$\endgroup\$ – Mathieu Guindon Jun 22 '18 at 14:02
  • 2
    \$\begingroup\$ Eh, coming up with a good CR title is rather hard actually. \$\endgroup\$ – Mathieu Guindon Jun 22 '18 at 14:44
  • 2
    \$\begingroup\$ Rolled back the code spacing edit - if the code wasn't indented by OP then it's something to address. \$\endgroup\$ – Raystafarian Jun 23 '18 at 4:51
2
\$\begingroup\$

ooh wee Because of the below statement, I'll do this in little steps which may not be the best approach, but should be easy to follow.

I don't understand the syntax of index match, otherwise I could have eliminated ...

let me drop an article from my website right here

I'm not bashing you for not knowing index/match, but you might as well learn it! use it to look up to the left

VBA and worksheet functions


All right, so you're using VBA - you don't need to use worksheet functions at all. If you need the function to exist because the audience needs to see the functions rather than the results, then that's another issue.

ActiveWorkbook.Sheets("Bulk Sheet").Range("H3:K3" & LrowBlk).SpecialCells(xlCellTypeVisible).Copy

Is this essentially .Range("H3:K3" 130) (using 130 arbitrarily)? How does that work?

You also sort based on H3:I3 but then copy H3:K3 and P - are columns J through P filtered when you filter based on column I?

That's pretty confusing. Essentially you have a range (like "H3:P400") that you want to get elements from only if columns 2 matches a string ("adGroup")?

So what? Arrays.


There are a lot of ways to do this, but let's start with the way you've done it, but instead use an array -

bulksheet.Range("H3:P" & lastRow).AutoFilter field:=2, Crteria1:="AdGroup"
Dim bulkData As Variant
bulkData = bulksheet.Range("H3:K" & lastRow).SpecialCells(xlCellTypeVisible)

Right now you have all your data in a variant where you can do your thing instead of on the sheet. Essentially, it's the same advice as

Be sure to avoid things like .Select - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this.

So now you have a variant where there are 9 columns and you need 4 of them on a new sheet. You could just put them on the new sheet like this -

For i = LBound(bulkData) To UBound(bulkData)
    adGroupSheet.Cells(i, 1) = bulkData(i, 1)
    adGroupSheet.Cells(i, 2) = bulkData(i, 2)
    adGroupSheet.Cells(i, 3) = bulkData(i, 3)
    adGroupSheet.Cells(i, 1) = bulkData(i, 9)     
Next

But maybe there's a better way to do all of that (I'm not 100%, I might have read somewhere that autofilter is faster than a loop, but I digress). Simply -

Dim bulkData As Variant
Dim bulkLastRow As Long
bulkLastRow = bulksheet.Cells(bulksheet.Rows.Count, 8).End(xlUp).Row
bulkData = bulksheet.Range(bulksheet.Cells(1, 8), bulksheet.Cells(lastrow, 16))
Dim sortedData As Variant
ReDim sortedData(1 To bulkLastRow, 1 To 4)
Dim bulkIndex As Long
Dim sortedIndex As Long
sortedIndex = 1
For bulkIndex = LBound(bulkData) To UBound(bulkData)
    If bulkData(bullkindex, 2) = "AdGroup" Then
        sortedData(sortedIndex, 1) = bulkData(bulkIndex, 1)
        sortedData(sortedIndex, 2) = bulkData(bulkIndex, 2)
        sortedData(sortedIndex, 3) = bulkData(bulkIndex, 3)
        sortedData(sortedIndex, 4) = bulkData(bulkIndex, 9)
        sortedIndex = sortedIndex + 1
    End If
Next

Right, now you have your sorted data in a variant and could easily just

 sortedsheet.Range(sortedsheet.Cells(1, 1), sortedsheet(sortedIndex, 4)) = sortedData

Now you can write your formulas.

But wait


But, I see now that you moved column 3 to column 4 -

With Sheets("Adgroup List")
    .Columns("C:C").Cut
    .Columns("E:E").Insert Shift:=xlToRight
 End With

This could easily be fixed by just putting it there in the first place with something like

 ReDim sortedData(1 To bulkLastRow, 1 To 5)
 sortedData(sortedIndex, 1) = bulkData(bulkIndex, 1)
 sortedData(sortedIndex, 2) = bulkData(bulkIndex, 2)
 sortedData(sortedIndex, 4) = bulkData(bulkIndex, 3)
 sortedData(sortedIndex, 5) = bulkData(bulkIndex, 9)

And you have a blank column.


All right, so?

But, why stop there? Assuming you don't need the formulas to be present on the sheet, you can get your data from your variant. But let's examine what you're doing first -

LrowSTR = termSheet.Cells(termSheet.Rows.Count, 8).End(xlUp).Row
Dim termData As Variant
ReDim termData(1 To 3)
termData(1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
termData(2) = "=VLOOKUP($K4,'Adgroup List'!$C:$E,3,0)"

You make a 1 dimensional array, look up a single value to populate 2 of the 3 items and never use your LrowSTR. Then

.Range("AJ4:AK4").Formula = Strformulas
.Range("AJ4:AK" & LrowSTR).FillDown

Yikes - of course that will hang up. Even without really changing your code it would be easier to just

For i = LBound(termData) To UBound(termData)
    termData(i, 1) = "=VLOOKUP($K4,'Adgroup List'!$C:$D,2,0)"
    termData(i, 2) = "=VLOOKUP($K&" & i + 3 & ",'Adgroup List'!$C:$E,3,0)"
Next

And then paste it all at once, no need to autofill (which is definitely slow). You're also searching column "C" when you already moved it and it's empty. Why not just look in D and E? I digress

Regardless, you might just

Dim termData As Variant
termData = termsheet.Range(termsheet.Cells(4, 11), termsheet(LrowSTR, 11))
ReDim Preserve termData(1 To UBound(termData), 1 To 3)
'find things in sortedData and populate termData with values
'print to termSheet

Misc

Other things to note -

  1. You've declared all of your variables, that's great! Your naming could be improved a bit and Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names. So LrowBlk would be bulkLastRow or something. And ws would be a terrible name, but

  2. Worksheets have a CodeName property - View Properties window (F4) and the (Name) field (the one at the top) can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.

  3. Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know. This isn't a problem for you, but it's good practice - I have it on.

  4. It's good practice to indent all of your code that way Labels will stick out as obvious. VBE standard is 4 spaces between levels

\$\endgroup\$
  • \$\begingroup\$ Hi Raystafarian. Thank you so much for answering my question and helping me understand the errors in my code. As a beginner who's only learnt VBA via youtube videos, I am extremely grateful for this in-depth response to my question. Answers such as these are a treasure trove of info for the uninitiated. Your website has some interesting links too and I will be going through that to learn VBA better. Thanks again! \$\endgroup\$ – anish rao Jun 24 '18 at 14:08
  • 1
    \$\begingroup\$ That's what it's all about - hopefully you gain something from this answer, and the next, and the next! \$\endgroup\$ – Raystafarian Jun 25 '18 at 6:30

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.