3
\$\begingroup\$

I have a spreadsheet with seven tables (tbl_1, tbl_2, ..., tbl_7) each controlled by its own slicer. Each slicer has six buttons (10, 20, 30, 40, 50, 60) referring to Team Codes. I use the code below to select one team on every slicer, then create a PDF for each team / slicer setting.

As of now, the code takes anywhere from 5-7min to run.

Sub SlicerTeam()
Dim wb As Workbook
Dim sc As SlicerCache
Dim si As SlicerItem

On Error GoTo errHandler
Application.ScreenUpdating = False
Application.EnableEvents = False

Set wb = ThisWorkbook

For x = 1 To 6
    For i = 1 To 7
    Set sc = wb.SlicerCaches("tbl_" & i)
        sc.ClearAllFilters
        For Each si In sc.VisibleSlicerItems
            Set si = sc.SlicerItems(si.Name)
                If Not si Is Nothing Then
                    If si.Name = x * 10 Then
                        si.Selected = True
                    Else
                        si.Selected = False
                    End If
                Else
                    si.Selected = False
                End If
        Next si

    Next i
Call PDFCreate
Next x

exitHandler:
Application.ScreenUpdating = True
Application.EnableEvents = True
Exit Sub

errHandler:
MsgBox ("Error in updating slicer filters.")
Resume exitHandler

End Sub

Sub PDFCreate()
Dim Fname As String
Dim path As String

path = "S:\MyFilePath\"
Fname = path & Sheets("Detail").Range("T1").Value & " - " & [TEXT('Population Detail'!B1,"mmm, yyyy")] & ".pdf"

    Sheets("Detail").ExportAsFixedFormat Type:=xlTypePDF, _
        FileName:=Fname, Quality:=xlQualityStandard, IncludeDocProperties:=True, _
        IgnorePrintAreas:=False, OpenAfterPublish:=False

End Sub
\$\endgroup\$
2
  • \$\begingroup\$ My bet is the Call PDFCreate is the bottleneck. What's that code? \$\endgroup\$ Feb 16, 2017 at 22:35
  • \$\begingroup\$ I have added the PDFCreate code as well. I have never had any speed issues with this subroutine. Whenever I step through the code, the problem seems to be more related to all of the loops. \$\endgroup\$
    – J Moore
    Feb 16, 2017 at 23:02

1 Answer 1

2
\$\begingroup\$

Variables

Let's discuss variables a bit.

  1. 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.

  2. When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type. Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

  3. Variable names - give your variables meaningful names.

  4. Personal preference - I avoid variables like i and j because they aren't descriptive. But, there's technically nothing wrong with using them as it's standard practice to iterate with i.

  5. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

That being said -

You don't declare x or i. But those aren't vary descriptive are they? Try:

Dim slicerButtonNumber As Long
Dim tableNumber As Long

You don't have great names for the variables you do declare -

wb - targetWorkbook
sc - targetSlicerCache
si - targetSlicerItem

Extending the names using descriptive words costs you nothing and saves you headaches!

Also, here:

Set targetSlicerCache = targetBook.SlicerCaches("tbl_" & tableNumber)

You're using a constant - "tbl_", so

Const TABLE_PREFIX as String = "tbl_"
Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)

It may not seem like much, but by declaring that at top, if it ever changes, you only need to change it in one place.

Same goes for PDFCreate:

Const SHEET_NAME As String = "Detail"
Const POPULATION_SHEET_NAME As String = "Population Detail"
Const SEPARATOR As String = " - "
Dim detailSheet As Worksheet
Set detailSheet = ThisWorkbook.Sheets(SHEET_NAME)
Dim populationSheet As Worksheet
Set populationSheet = ThisWorkbook.Sheets(POPULATION_SHEET_NAME)
Dim file_Name As String
Dim file_Path As String
Dim aPathValue As String

file_Path = "S:\MyFilefilePath\"
aPathValue = detailSheet.Range("T1").Value & SEPARATOR
file_Path = file_Path & aPathValue
file_Name = filePath & [TEXT(populationsheet.range("B1"),"mmm, yyyy")] & ".pdf"

detailSheet.ExportAsFixedFormat Type:=xlTypePDF, _
    filename:=file_Name, Quality:=xlQualityStandard, IncludeDocProperties:=True, _
    IgnorePrintAreas:=False, OpenAfterPublish:=False

You don't need to Call subs, it's obsolete. Instead just use Sub argument, argument


Speed

Your set here is redundant

        For Each targetSlicerItem In sc.VisibleSlicerItems
            Set targetSlicerItem = targetSlicerCache.SlicerItems(targetSlicerItem.Name)

The For sets the items.

This is also unnecessary:

If Not targetSlicerItem Is Nothing Then

It won't be Nothing since you're only addressing the items that exist in the cache.

You can condense down

For slicerButtonNumber = 1 To 6
    For tableNumber = 1 To 7
    Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)
        targetSlicerCache.ClearAllFilters
        For Each targetSlicerItem In targetSlicerCache.VisibleSlicerItems
            If targetSlicerItem.Name = slicerButtonNumber * 10 Then
                targetSlicerItem.Selected = True
            Else
                targetSlicerItem.Selected = False
        End If
        Next targetSlicerItem
    Next i

You can however, remove one of your loops, the slicerButtonNumber loop

Const BUTTON_NUMBERS As String = "10,20,30,40,50,60"
Const DELIMITER As String = ","
Dim slicerButtonNumbers As Variant
slicerButtonNumbers = Split(BUTTON_NUMBERS, DELIMITER)
    

 For tableNumber = 1 To 7
    Set targetSlicerCache = targetBook.SlicerCaches(TABLE_PREFIX & tableNumber)
        targetSlicerCache.ClearAllFilters
        For Each targetSlicerItem In targetSlicerCache.VisibleSlicerItems
            If UBound(Filter(slicerButtonNumbers, targetSlicerItem.Name)) > -1 Then
               targetSlicerItem.Selected = True
            Else
                targetSlicerItem.Selected = False
            End If
        Next targetSlicerItem
    Next tableNumber
\$\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.