Print worksheets depending on checkbox. Slower than before

I have an userform that has check boxes where user can decide which worksheets to print.

Code in userform:

Sub PrintOK_Click()

PrintSelection = Summary.value + _
CCSummary.value + _
DivSalaries.value + _
CCSalaries.value + _
PrintAll.value _

' Displays warning box if no report is selected
If PrintSelection = 0 Then
    MsgBox "No Report(s) selected. Please select Report(s) to print.", 0 + vbExclamation, "Incomplete Request"
End If

' Check to see which worksheets have been selected and
' Run print modules for selected worksheets
    If Summary.value = -1 Then
        Call setup("Summary")
    End If

    If CCSummary.value = -1 Then
        Call setup("Monthly CC Sum")
    End If

    If DivSalaries.value = -1 Then
        Call setup("Div Sal Sum")
    End If

    If CCSalaries.value = -1 Then
        Call setup("CC Sal Sum")
    End If

    If PrintAll.value = -1 Then
        Call setup("PrintAll")
    End If

' Close Form box
End
End Sub

Userform selection is passed into the following. Takes the worksheet(s) selected and stores into array.

Sub setup(Optional tabs As String)
'store sheets into array

Dim arr() As Variant

'called from printer icon on activesheet
If tabs = "" Then
    arr = Array(ActiveSheet.Name)

'called via checkbox from print menu
ElseIf tabs = "PrintAll" Then
    arr = Array("Summary", "Monthly CC Sum", "Div Sal Sum", "CC Sal Sum")
Else
    arr = Array(tabs)
End If

Call goPrint(arr)

End Sub

Which then passes the array to another sub to do the printing

```
Sub goPrint(tabArr As Variant)

Dim ws As Worksheet
Dim area As String
Dim title As String

'loop through array to print each sheet
For i = 0 To UBound(tabArr)

Set ws = Worksheets(tabArr(i))

With ws

If .Name = "Summary" Then
area = "PRNSUMMARY"
title = "$1:$9"
ElseIf .Name = "Monthly CC Sum" Then
area = "PRNMONTHLYCCSUM"
title = "$2:$16"
ElseIf .Name = "Div Sal Sum" Then
are

(Previous code had a different sub for each worksheet thus repeating the pagesetup code block multiple times.)

Was the previous code also working directly with Worksheet objects by any chance?

You're working with an array of strings / sheet names...

'called from printer icon on activesheet
If tabs = "" Then
    arr = Array(ActiveSheet.Name)

'called via checkbox from print menu
ElseIf tabs = "PrintAll" Then
    arr = Array("Summary", "Monthly CC Sum", "Div Sal Sum", "CC Sal Sum")
Else
    arr = Array(tabs)
End If

...and then creating worksheet objects as you iterate the array:

'loop through array to print each sheet
For i = 0 To UBound(tabArr)

    Set ws = Worksheets(tabArr(i))

Why not work with an array of Worksheet objects instead, and simply iterate that array?

If tabs = vbNullString then
    arr = Array(ActiveSheet)
ElseIf tabs = "PrintAll" then
    arr = Array(SummarySheet, MonthlySummary, DivSalSumSheet, CCSalSumSheet)
Else
    sheetNames = Split(tabs, ",")
    arrLength = UBound(sheetNames)
    ReDim arr(0 To arrLength)
    For i = 0 To arrLength
        arr(i) = ThisWorkbook.Worksheets(sheetNames(i))
    Next
End If

That's still brittle though - ideally you'd have the worksheet references themselves handy, ready to be passed to that goPrint method.

The goPrint method could then simply iterate the object array with a simple For Each:

For Each ws In tabArr

This code isn't ideal either; your previous code was also a little faster because it didn't need to check this condition:

If .Name = "Summary" Then
        area = "PRNSUMMARY"
        title = "$1:$9"
    ElseIf .Name = "Monthly CC Sum" Then
        area = "PRNMONTHLYCCSUM"
        title = "$2:$16"
    ElseIf .Name = "Div Sal Sum" Then
        area = "PRNDIVSALSUM"
        title = ""
    ElseIf .Name = "CC Sal Sum" Then
        area = "PRNCCSALSUM"
        title = "$2:$16"
    End If

If the sheet name is CC Sal Sum then you accessed and compared the worksheet's name 4 times already. And if it's something else, ...you get whatever you get.

This would be a nice case for a Select Case statement:

Select Case .Name
    Case "Summary"
        '...
    Case "Monthly CC Sum"
        '...
    Case "Div Sal Sum"
        '...
    Case "CC Sal Sum"
        '...
    Case Else
        'handle unexpected input?
 End Select

At runtime, a Select Case block pretty much amounts to a conditional GoTo jump, which means no matter how many cases you have, you'll only ever evaluate the condition once.

A note about this:

' Close Form box
End

That's the big red "NUKE 'EM ALL" button. Sure, End will close the form box. But it will also end the macro. It's very very very rare that you actually need to do this.

Compare to:

'Close form box
Me.Hide

Is that comment even needed then?

Structurally, the main issue I'm seeing is that the code is relying on side-effects: the form calls setup, and then closes. What's setup doing then? Shouldn't it set things up for printing?

Oh, it does... but it's also calling goPrint - which is a side-effect that makes it much harder to write a test for the method and see if it works as intended. Everything is coupled, and the UI is running the show.

What if you flipped things around instead, and controlled things from the outside?

Sub PromptAndPrint()

    Dim model As New SelectionModel
    model.Selection = "PrintAll"

    Dim view As New SelectionForm
    Set view.ViewModel = model
    view.Show vbModal

    If view.IsCancelled Then Exit Sub

    ConfigurePageSetup model.SelectedWorksheets
    PrintSheets model.SelectedWorksheets

End Sub

That's right: the UI becomes nothing more than a presentation tool; the execution flow is controlled by the macro, not by the UI, and then you have some SelectionModel class responsible for encapsulating the data you want to pass around, and abstract away the sheet names -> worksheet objects transformation; you can test ConfigurePageSetup without wasting a sheet of paper, and you can test the whole SelectionModel functionality without even showing the UI, without configuring page setup and without wasting a sheet of paper: VBA is much more object-oriented than people generally want to admit.

Nitpicks:

• Indentation isn't always consistent. Consider using VBE add-ins like the latest MZ-Tools or Rubberduck, which offer automatic indentation (disclaimer: I'm heavily involved in Rubberduck).

• Member names don't follow common naming conventions. Use PascalCase, not camelCase, for public members; that's consistent with every single type library and object model available to VBA.

Also:

• Consider using explicit access modifiers. Procedures are implicitly Public by default.

• Consider using explicit parameter modifiers. Parameters are passed ByRef unless specified otherwise.

• Consider extracting the margins to a user-defined type, or at least a few constants.

• Consider re

StackExchange Code Review Q#129856, answer score: 7