Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I maintain a VBA process that prints a large number of PDFs. It was awful when I inherited it and I've been making improvements over time, but I'm a bit stuck on this one. There are at least 4 functions with this exact same code used to check to see if the program has been initialized properly. They could all be called individually, or together depending on circumstances.

I do not want to initialize again if I don't have to. Hence the check in every subroutine, but I just can't figure out how to do it any differently. Thoughts? Is there a design pattern to deal with this kind of thing that I don't know of?

Please try to focus on the pattern and not the supporting intitializer. I know there are a few issues there and intend to deal with them soon. I included it in case it was relevant.

Main code:

Private rootDir As String
Private isInit As Boolean
Private mCycle As String

Public Function printPDF_VoidedNewItems()
    If Not isInit Then
        initialize
    End If
    If rootDir <> "" And mCycle <> "" Then
        If setPrinterToPDF() Then
            ' logic specific to printing this pdf
        End If
    End If
End Function

Public Function printPDF_ComsNotFed()
    If Not isInit Then
        initialize
    End If
    If rootDir <> "" And mCycle <> "" Then
        If setPrinterToPDF() Then
            ' logic specific to printing this pdf
        End If
    End If
End Function

Initializer:

Private Sub initialize()
    rootDir = folderPicker() & "\"
    mCycle = getCycleInput
    isInit = True
End Sub

Private Function folderPicker() As String
'*******************************************
' returns directory path to be printed to
'   does not allow multiple selections,
'   so returning the first item in selected
'   items is sufficient.
'
' returns empty string On Error or if the
'   user cancels
'********************************************
On Error GoTo ErrHandler

    Dim folderChosen As Integer
    Dim fd As Variant
    Set fd = Application.FileDialog(msoFileDialogFolderPicker)
    With fd
        .AllowMultiSelect = False
        .Title = "Choose Directory to Print to"
        .InitialFileName = "\\server\share\"
        .InitialView = msoFileDialogViewSmallIcons
        folderChosen = .Show
        If folderChosen <> -1 Then
            folderPicker = ""
        Else
            folderPicker = .SelectedItems(1)
        End If
    End With
Exit Function

ErrExit:
    folderPicker = ""
    Exit Function
ErrHandler:
    MsgBox "Unexpected Error: " & Err.number & vbCrLf & "Source: " & Err.Source & _
        "Description: " & Err.Description, vbCritical, "ERROR!"
End Function

Private Function getCycleInput() As String
' Displays a form with a combo box to the user.
' Returns an empty string if canceled or errored
'   Otherwise returns 8 character Cycle from dbo.MAM_ISE_SCHEDULE
On Error GoTo ErrHandler

    Dim cpkr As New Form_CyclePicker

    'wait for user input
    Do Until cpkr.Cancel = True Or cpkr.Confirmed = True 'Do Until user clicks "OK" or "Cancel"
        DoEvents 'frees up OS to actually display form and allow user to interact with it
    Loop

    If cpkr.Cancel Or Not cpkr.Confirmed Then ' not confirmed seems redundant, but certainly can't hurt to check.
        ' return
        getCycleInput = ""
        GoTo ExitSub
    Else
        ' return
        getCycleInput = cpkr.CycleNumber
    End If

ExitSub:
    Set cpkr = Nothing
    Exit Function
ErrHandler:
    MsgBox Err.Description, vbCritical, "Runtime Error: " & Err.number
    getCycleInput = ""
    Resume ExitSub
End Function
share|improve this question
    
Is the main code in a class module? –  Mat's Mug Jul 2 at 15:45
    
It probably should be (somehow), but no. It's in a regular .bas module. –  ckuhn203 Jul 2 at 15:46
    
I think it would be best to include the parts you replaced with comments, so we can see how much duplication is really going on. –  Mat's Mug Jul 2 at 15:55
    
I'm already refactoring the duplication inside of them into their own functions and routines @Mat'sMug. I'd like to focus on this pattern. It's the part I don't know how to deal with. –  ckuhn203 Jul 2 at 15:58

2 Answers 2

up vote 3 down vote accepted

your Main code is split into two functions right now, which only differ in the logic inside the nested if statement, you should do the if statement and initializing outside the function and just

If Not isInit Then
    initialize
End If
If rootDir <> "" And mCycle <> "" Then
    If setPrinterToPDF() Then
        ' Print VoidedNewItems
        ' Print ComsNotFed
    End If
End If

make the functions the guts in the middle and then call them inside one if statement like above.

Public Function printPDF_VoidedNewItems()
    ' logic specific to printing this pdf
End Function

then call the function like this.

If Not isInit Then
    initialize
End If
If rootDir <> "" And mCycle <> "" Then
    If setPrinterToPDF() Then
        printPDF_VoidedNewItems()
        printPDF_ComsNotFed()
    End If
End If

that way you are only doing one check on everything instead of two when you call these functions

If Not isInit Then
    initialize
End If
If rootDir <> "" And mCycle <> "" And setPrinterToPDF() Then
    printPDF_VoidedNewItems()
    printPDF_ComsNotFed()
End If

they are all getting checked the same might as well have all the checks in one place.

share|improve this answer
    
That is a good idea, but how does the function tell which one to actually print now? –  Mat's Mug Jul 2 at 16:00
1  
But I can call them independently as well... Maybe I could add a Enum ReportType and a parameter to PrintPDF(type as ReportType")?? –  ckuhn203 Jul 2 at 16:01
    
@ckuhn203 that is a good idea –  Malachi Jul 2 at 16:02
    
alternatively UBound(Filter(Array(rootDir, mCycle), vbNullString)) > -1 –  me how Jul 29 at 10:13
    
@mehow, I am a little lost here could you please clarify what you are saying a little bit, it's been a while since I wrote this review... –  Malachi Jul 29 at 13:05

Down with the underscore

I find the naming of printPdf_xxxx functions confusing, because if that code is/were in a class module they should be private, and the class would say Implements printPdf, where printPdf would be a class with VoidedNewItems and ComsNotFed members... or it could be that there's a private field called printPdf that's declared WithEvents and the functions are event handlers made public.

Avoid underscores in procedure & function (/method) names, the language has a meaning for identifiers with an underscore.


Initializing once

If you scratch that code and implement it in a class instead, and run initialisation in the... Class_Initialize method, which can be thought of as a parameterless constructor that runs when you create the object. Not too crazy about spawning a UI prompt in that handler though, but then the methods wouldn't have to worry about whether or not Initialize has executed.

share|improve this answer
    
So, if I implement this in a class, I would know already that it's initialized, so there'd be no need to check at all. Nice. Thanks. I should have thought of that. –  ckuhn203 Jul 2 at 16:14
    
Yes. The not-so-great part of it is that you now have the side effect of a UI prompt with every new instantiation of the class though. –  Mat's Mug Jul 2 at 16:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.