4
\$\begingroup\$

Objective:

Have a single point of entrance to initialize a class that holds instances of "sub" classes


Background:

  • I read about inheritance in VBA (or as close as you will) in this post and it occurred to me that you could have a hierarchy of classes/objects so accessing them while coding would be easier
  • Thought about how to initialize the "base" class in order to recover from code crashes in this post
  • Read about Lazy object / weak reference in this post and got worried about this could be happening in my project

Questions:

  1. Is this a source of a memory leak risk? (see my post about the idea of a framework here where I'm asking from a different perspective because in this case I am creating a new instance of the base class inside the function)
  2. The idea of a single point of entrance to initialize the class is "compatible" / recommeded with the whole classes hierarchy approach?

Code / File structure:

Code file structure


Code components:

Module: AppMacros

Description: This is the function that I call to access the classes in the whole application.

'@Version(1)
'@Folder("App")
Option Explicit

Private Const ModuleName As String = "AppMacros"

Public Function AppWorkbook() As App

    On Error GoTo CleanFail

    Static newApp As App

    If newApp Is Nothing Then
        Set newApp = App.Create
        LogManager.Register TableLogger.Create("AppInfoLogger", PerfLevel, "TablaRegistroApp")
        newApp.SecurityManager.RestoreSheetsProtection
        newApp.SecurityManager.RestoreWorkbookProtection
    End If

    Set AppWorkbook = newApp

CleanExit:
    Exit Function

CleanFail:
    ErrorHandler.DisplayMessage ModuleName, "InitAppWorkbook", Err.Number, Err.Description, , True
    If Not DebugMode Then Resume CleanExit Else: Stop: Resume

End Function

I call it like this:

How to call the point of entrance function

And call a function from one of the sub classes:

Call a function from one of the sub classes

Class: App

'@Version(1)
'@Folder("App")

Option Explicit
'@PredeclaredId

' Copywrite (C) 2019 Ricardo Diaz
' This file is distributed under the GPL-3.0 license
' Obtain a copy of the GPL-3.0 license <http://opensource.org/licenses/GPL-3.0>

Private Type TApp
    DateUpdated As Date

    AutomationManager As AutomationManager
    ConfigManager As ConfigManager
    DisplayManager As DisplayManager
    ExternalDataManager As ExternalDataManager
    ErrorHandler As ErrorHandler
    NavigationManager As NavigationManager
    OptionsManager As OptionsManager
    ParamManager As ParamManager
    PerfManager As PerfManager
    RoadMapManager As RoadMapManager
    SecurityManager As SecurityManager
    SettingsManager As DefaultsManager
    StartManager As StartManager
    StateManager As StateManager
    TaskManager As TaskManager
    VersionManager As VersionManager
End Type

Private this As TApp

Public Property Get DateUpdated() As Date
    DateUpdated = this.DateUpdated
End Property

Public Property Let DateUpdated(ByVal vNewValue As Date)
    this.DateUpdated = vNewValue
End Property

Public Property Get Self() As App
    Set Self = Me
End Property

Public Property Get AutomationManager() As AutomationManager
    Set AutomationManager = this.AutomationManager
End Property

Friend Property Set AutomationManager(ByVal Value As AutomationManager)
    Set this.AutomationManager = Value
End Property

Public Property Get ConfigManager() As ConfigManager
    Set ConfigManager = this.ConfigManager
End Property

Friend Property Set ConfigManager(ByVal Value As ConfigManager)
    Set this.ConfigManager = Value
End Property

Public Property Get DisplayManager() As DisplayManager
    Set DisplayManager = this.DisplayManager
End Property

Friend Property Set DisplayManager(ByVal Value As DisplayManager)
    Set this.DisplayManager = Value
End Property

Public Property Get ErrorHandler() As ErrorHandler
    Set ErrorHandler = this.ErrorHandler
End Property

Friend Property Set ErrorHandler(ByVal Value As ErrorHandler)
    Set this.ErrorHandler = Value
End Property

Public Property Get ExternalDataManager() As ExternalDataManager
    Set ExternalDataManager = this.ExternalDataManager
End Property

Friend Property Set ExternalDataManager(ByVal Value As ExternalDataManager)
    Set this.ExternalDataManager = Value
End Property

Public Property Get NavigationManager() As NavigationManager
    Set NavigationManager = this.NavigationManager
End Property

Friend Property Set NavigationManager(ByVal Value As NavigationManager)
    Set this.NavigationManager = Value
End Property

Public Property Get OptionsManager() As OptionsManager
    Set OptionsManager = this.OptionsManager
End Property

Friend Property Set OptionsManager(ByVal Value As OptionsManager)
    Set this.OptionsManager = Value
End Property

Public Property Get ParamManager() As ParamManager
    Set ParamManager = this.ParamManager
End Property

Friend Property Set ParamManager(ByVal Value As ParamManager)
    Set this.ParamManager = Value
End Property

Public Property Get PerfManager() As PerfManager
    Set PerfManager = this.PerfManager
End Property

Friend Property Set PerfManager(ByVal Value As PerfManager)
    Set this.PerfManager = Value
End Property

Public Property Get RoadMapManager() As RoadMapManager
    Set RoadMapManager = this.RoadMapManager
End Property

Friend Property Set RoadMapManager(ByVal Value As RoadMapManager)
    Set this.RoadMapManager = Value
End Property

Public Property Get SecurityManager() As SecurityManager
    Set SecurityManager = this.SecurityManager
End Property

Friend Property Set SecurityManager(ByVal Value As SecurityManager)
    Set this.SecurityManager = Value
End Property

Public Property Get SettingsManager() As DefaultsManager
    Set SettingsManager = this.SettingsManager
End Property

Friend Property Set SettingsManager(ByVal Value As DefaultsManager)
    Set this.SettingsManager = Value
End Property

Public Property Get StartManager() As StartManager
    Set StartManager = this.StartManager
End Property

Friend Property Set StartManager(ByVal Value As StartManager)
    Set this.StartManager = Value
End Property

Public Property Get StateManager() As StateManager
    Set StateManager = this.StateManager
End Property

Friend Property Set StateManager(ByVal Value As StateManager)
    Set this.StateManager = Value
End Property

Public Property Get TaskManager() As TaskManager
    Set TaskManager = this.TaskManager
End Property

Friend Property Set TaskManager(ByVal Value As TaskManager)
    Set this.TaskManager = Value
End Property

Public Property Get VersionManager() As VersionManager
    Set VersionManager = this.VersionManager
End Property

Friend Property Set VersionManager(ByVal Value As VersionManager)
    Set this.VersionManager = Value
End Property

'@Ignore FunctionReturnValueNotUsed
Public Function Create() As App
    With New App
        Set .AutomationManager = New AutomationManager
        Set .ConfigManager = New ConfigManager
        Set .DisplayManager = New DisplayManager
        Set .ErrorHandler = New ErrorHandler
        Set .ExternalDataManager = New ExternalDataManager
        Set .NavigationManager = New NavigationManager
        Set .OptionsManager = New OptionsManager
        Set .ParamManager = New ParamManager
        Set .PerfManager = New PerfManager
        Set .RoadMapManager = New RoadMapManager
        Set .SecurityManager = New SecurityManager
        Set .SettingsManager = New DefaultsManager
        Set .StartManager = New StartManager
        Set .StateManager = New StateManager
        Set .TaskManager = New TaskManager
        Set .VersionManager = New VersionManager
        .VersionManager.Create

        Set Create = .Self
    End With
End Function

Class (sub-class): ExternalDataManager

'@Version(2 )
'@Folder("App.ExternalData")

Option Explicit
'@PredeclaredId

' Copywrite (C) 2019 Ricardo Diaz
' This file is distributed under the GPL-3.0 license
' Obtain a copy of the GPL-3.0 license <http://opensource.org/licenses/GPL-3.0>

'
' Private Members
' ---------------
'

'
' Public Members
' --------------
'

'
' Private Methods
' ---------------
'
Private Function DoesQueryExist(ByVal QueryName As String) As Boolean
    ' Helper function to check if a query with the given name already exists
    Dim evalQuery As WorkbookQuery

    If (ThisWorkbook.Queries.Count = 0) Then
        DoesQueryExist = False
        Exit Function
    End If

    For Each evalQuery In ThisWorkbook.Queries
        If (evalQuery.Name = QueryName) Then
            DoesQueryExist = True
            Exit Function
        End If
    Next
    DoesQueryExist = False
End Function

Private Sub RefreshQueryWaitUntilFinish(ByVal currentConnection As WorkbookConnection)

    Dim backgroundRefresh As Boolean

    With currentConnection.OLEDBConnection
        backgroundRefresh = .BackgroundQuery
        .BackgroundQuery = False
        .Refresh
        .BackgroundQuery = backgroundRefresh
    End With

End Sub

Private Sub LoadToWorksheetOnly(ByVal query As WorkbookQuery, ByVal loadSheet As Worksheet)
    ' The usual VBA code to create ListObject with a Query Table
    ' The interface is not new, but looks how simple is the connection string of Power Query:
    ' "OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location=" & query.Name

    With loadSheet.ListObjects.Add(SourceType:=0, source:= _
        "OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location=" & query.Name _
        , destination:=Range("$A$1")).queryTable
        .CommandType = xlCmdDefault
        .CommandText = Array("SELECT * FROM [" & query.Name & "]")
        .RowNumbers = False
        .FillAdjacentFormulas = False
        .PreserveFormatting = True
        .RefreshOnFileOpen = False
        .BackgroundQuery = True
        .RefreshStyle = xlInsertDeleteCells
        .SavePassword = False
        .SaveData = True
        .AdjustColumnWidth = True
        .RefreshPeriod = 0
        .PreserveColumnInfo = False
        .Refresh BackgroundQuery:=False
    End With

End Sub

Private Sub LoadToWorksheetAndModel(ByVal query As WorkbookQuery, ByVal currentSheet As Worksheet)
    ' Let's load the query to the Data Model
    LoadToDataModel query

    ' Now we can load the data to the worksheet
    With currentSheet.ListObjects.Add(SourceType:=4, source:=ActiveWorkbook. _
        Connections("Query - " & query.Name), destination:=Range("$A$1")).TableObject
        .RowNumbers = False
        .PreserveFormatting = True
        .PreserveColumnInfo = False
        .AdjustColumnWidth = True
        .RefreshStyle = 1
        .ListObject.DisplayName = Replace(query.Name, " ", "_") & "_ListObject"
        .Refresh
    End With
End Sub

Private Sub LoadToDataModel(ByVal query As WorkbookQuery)

    ' This code loads the query to the Data Model
    ThisWorkbook.Connections.Add2 "Query - " & query.Name, _
        "Connection to the '" & query.Name & "' query in the workbook.", _
        "OLEDB;Provider=Microsoft.Mashup.OleDb.1;Data Source=$Workbook$;Location=" & query.Name _
        , """" & query.Name & """", 6, True, False

End Sub
'@Ignore ProcedureNotUsed
Private Sub ReplaceStringInWorkBook(ByVal SearchFor As String, ByVal ReplaceWith As String)

    Dim evalQuery As WorkbookQuery

    For Each evalQuery In ThisWorkbook.Queries

        ReplaceStringInQuery evalQuery.Name, SearchFor, ReplaceWith

    Next evalQuery

End Sub

Private Sub ReplaceStringInQuery(ByVal QueryName As String, ByVal SearchFor As String, ByVal ReplaceWith As String)

    Dim queryFormula As String
    Dim queryResult As String

    If DoesQueryExist(QueryName) = False Then Exit Sub

    queryFormula = ThisWorkbook.Queries(QueryName).Formula

    queryResult = Replace(queryFormula, SearchFor, ReplaceWith)

    ThisWorkbook.Queries(QueryName).Formula = queryResult

End Sub

'@Ignore ProcedureNotUsed, AssignedByValParameter
Public Sub TransferQueries(Optional ByVal FromWorkbook As Workbook, Optional ByVal ToWorkbook As Workbook, Optional ByVal overwrite As Boolean = False)

    Dim sourceQuery As WorkbookQuery

    If FromWorkbook Is Nothing Then Set FromWorkbook = Application.ThisWorkbook
    If ToWorkbook Is Nothing Then Set ToWorkbook = Application.ActiveWorkbook
    If FromWorkbook.fullName = ToWorkbook.fullName Then Exit Sub

    For Each sourceQuery In FromWorkbook.Queries
        If QueryExists(sourceQuery.Name, ToWorkbook) Then
            If overwrite Then
                ToWorkbook.Queries(sourceQuery.Name).Delete
                ToWorkbook.Queries.Add sourceQuery.Name, sourceQuery.Formula, sourceQuery.Description
            End If
        Else
            ToWorkbook.Queries.Add sourceQuery.Name, sourceQuery.Formula, sourceQuery.Description
        End If
    Next
End Sub

' check if a given query exists in the given workbook
'@Ignore ProcedureNotUsed, AssignedByValParameter
Private Function QueryExists(ByVal EvalQueryName As String, Optional ByVal evalWorkbook As Workbook) As Boolean
    If evalWorkbook Is Nothing Then Set evalWorkbook = Application.ActiveWorkbook
    On Error Resume Next
    QueryExists = CBool(Len(evalWorkbook.Queries(EvalQueryName).Name))
    On Error GoTo 0
End Function

'
'
' Constructors
' ------------
'

'
' Class
' -----
'
' Private Sub Class_Initialize() : End Sub

'
' Enumerator
' Public Property Get NewEnum() As IUnknown : Attribute NewEnum.VB_UserMemId = -4 : Set NewEnum = pCollec.[_NewEnum] : End Property
'

' Public Methods
' --------------
'
Public Sub DisplayQueriesPane(ByVal Show As Boolean)
    Application.CommandBars("Queries and Connections").visible = Show
    Application.CommandBars("Queries and Connections").Width = 300
End Sub

'@Ignore ProcedureNotUsed
Public Sub ToggleQueriesPane()
    Application.CommandBars("Queries and Connections").visible = _
                Not (Application.CommandBars("Queries and Connections").visible)
    Application.CommandBars("Queries and Connections").Width = 300
End Sub
Public Sub UpdateAll()
    AppWorkbook.StateManager.DisplayStatusBarMessage True, , "(Actualizando todas las conexiones de datos)"
    ThisWorkbook.RefreshAll
    AppWorkbook.StateManager.DisplayStatusBarMessage True, , "(Actualización de todas las conexiones de datos finalizada)"
End Sub
Public Sub UpdateDataModel()
    AppWorkbook.StateManager.DisplayStatusBarMessage True, , "(Inicializando modelo de datos)"
    ThisWorkbook.Model.Initialize
    AppWorkbook.StateManager.DisplayStatusBarMessage True, , "(Modelo de datos inicializado, actualizando)"
    ThisWorkbook.Model.Refresh
    AppWorkbook.StateManager.DisplayStatusBarMessage True, , "(Actualización del modelo de datos finalizada)"
End Sub

Public Sub Update(Optional ByVal QueryName As String)

    Dim currentConnection As WorkbookConnection

    For Each currentConnection In ThisWorkbook.Connections

        Select Case QueryName <> vbNullString
        Case True
            If InStr(currentConnection.Name, QueryName) > 0 Then RefreshQueryWaitUntilFinish currentConnection

        Case False
            RefreshQueryWaitUntilFinish currentConnection

        End Select

    Next currentConnection

End Sub

'Refresh particular PowerPivot table
'@Ignore ProcedureNotUsed
Public Sub UpdatedPowerPivotTable(ByVal QueryName As String)
    ThisWorkbook.Model.Initialize
    ThisWorkbook.Connections(QueryName).Refresh
End Sub

' Credits from here under: https://gallery.technet.microsoft.com/office/VBA-to-automate-Power-956a52d1
' Adapted by Ricardo Diaz
Public Sub DeleteQuery(ByVal QueryName As String)

    Dim evalQuery As WorkbookQuery

    ' We get from the first worksheets all the data in order to know which query to delete, including its worksheet, connection and Data Model is needed

    Dim evalConnection As WorkbookConnection
    Dim connectionString As String

    For Each evalConnection In ThisWorkbook.Connections
        If Not evalConnection.InModel Then
            ' This is not a Data Model conenction. We created this connection without the "Power Query - " prefix, to determine if we should delete it, let's check the connection string
            If Not IsNull(evalConnection.OLEDBConnection) Then
                ' This is a OLEDB Connection. Good chance it is our connection. Let's check the connection string
                connectionString = evalConnection.OLEDBConnection.Connection
                Dim prefix As String
                prefix = "Provider=Microsoft.Mashup.OleDb.1;"
                If (Left$(connectionString, Len(prefix)) = prefix) And (0 < InStr(connectionString, "Location=" & QueryName)) Then
                    ' This is our connection
                    ' It starts with "Provider=Microsoft.Mashup.OleDb.1;" and contains "Location=" with our query name. This is our connection.
                    evalConnection.Delete
                End If
            End If
        ElseIf (InStr(1, evalConnection.Name, "Query - " & QueryName)) Then
            ' We created this connection with "Power Query - "  prefix, so we can this connection
            evalConnection.Delete
        End If
    Next

    If DoesQueryExist(QueryName) Then
        ' Deleting the query
        Set evalQuery = ThisWorkbook.Queries(QueryName)
        evalQuery.Delete
    End If

End Sub

' In parameters if not used "" rather vbNullString adding the query raises an error
'@Ignore ProcedureNotUsed, EmptyStringLiteral
Public Sub CreateQuery(ByVal QueryName As String, ByVal codeM As String, Optional ByVal shouldLoadToDataModel As Boolean = False, Optional ByVal shouldLoadToWorksheet As Boolean = False, Optional ByVal queryDescription As String = "")

    Dim evalQuery As WorkbookQuery
    Dim currentSheet As Worksheet

    If DoesQueryExist(QueryName) Then
        DeleteQuery QueryName
    End If

    ' The new interface to create a new Power Query query. It gets as an input the query name, codeM formula and description (if description is empty, th
    Set evalQuery = ThisWorkbook.Queries.Add(QueryName, codeM, queryDescription)



    If shouldLoadToWorksheet Then
        ' We add a new worksheet with the same name as the Power Query query
        Set currentSheet = ThisWorkbook.Sheets.Add(After:=ActiveSheet)
        currentSheet.Name = QueryName

        If Not shouldLoadToDataModel Then
            ' Let's load to worksheet only
            LoadToWorksheetOnly evalQuery, currentSheet
        Else
            ' Let's load to worksheet and Data Model
            LoadToWorksheetAndModel evalQuery, currentSheet
        End If
    ElseIf shouldLoadToDataModel Then
        ' No need to load to worksheet, only Data Model
        LoadToDataModel evalQuery
    End If

End Sub

Public Sub CreateNameParameterQueriesFromRange(ByVal EvalRange As Range)
    Dim EvalCell As Range

    For Each EvalCell In EvalRange.Cells

        CreateNameParameterQueryFromCell EvalCell

    Next EvalCell
End Sub


Public Sub CreateNameParameterQueryFromCell(ByVal CurrentCell As Range)

    If Framework.Name.DoesNameExists(CurrentCell) = False Then Exit Sub

    Dim QueryName As String
    Dim baseCode As String
    Dim wrapCode As String
    Dim cellStyleType As Long

    QueryName = CurrentCell.Name.Name
    baseCode = "fnCargarParamExcel(""<cellName>"", <cellStyleType>) meta [IsParameterQuery=true, Type=""Number"", IsParameterQueryRequired=false]"

    ' Cells style types defined in Styles classes               TODO: decouple class from constants
    ' 1 Text
    ' 2 Number
    ' 3 Date
    Select Case True
    Case InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsDate)
        cellStyleType = 3

    Case InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsYear), InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsNumber), InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsCurrency), InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsMultiple), InStr(LCase$(CurrentCell.style.Name), constStyleNameContainsPercentage)
        cellStyleType = 2

    Case Else
        cellStyleType = 1

    End Select

    '@Ignore AssignmentNotUsed
    wrapCode = Replace(baseCode, "<cellName>", QueryName)

    wrapCode = Replace(wrapCode, "<cellStyleType>", cellStyleType)

    CreateQuery QueryName, wrapCode, False, False

End Sub

Code has annotations from Rubberduck add-in

\$\endgroup\$
7
  • 2
    \$\begingroup\$ I'm not entirely sure what you want reviewed here; just your two questions, or the partial code you've posted? Regarding those questions in brief: 1) No there's no risk of memory leak here; VBA is a COM based language meaning it uses (strong) reference counting to keep objects alive. As long as the number of references to an object (variables pointing to it or With blocks) is non-zero, the object will hang around. The memory leak you're worried about here would be a circular reference, where a parent class holds a reference to a child class and the child holds a reference to the parent ... \$\endgroup\$
    – Greedo
    Jan 11 '20 at 12:21
  • 2
    \$\begingroup\$ ... e.g ExternalDataManager has a reference to App. In that case, if you create a New App (+1 reference) then later it falls out of scope (-1 reference), the App object will not be destroyed since its child subclass still holds a reference to it, and the child won't be destroyed since the parent still holds a reference to it. You never pass Me to the subclasses so there should be no circular references. As for weak references, these are where you save a pointer and dereference it - not native VBA so you'd know if you were doing it! Q. 2) is more nuanced so I'll leave for reviewers \$\endgroup\$
    – Greedo
    Jan 11 '20 at 12:33
  • 1
    \$\begingroup\$ Thank you Greedo for your time and excellent explanations. I've noticed that sometimes I close Excel and the instance remains in the task manager. So maybe there is a class instance "alive" and I suspect that the circular reference may be happening. As the code is very extensive, don't know well how to debug it. \$\endgroup\$ Jan 11 '20 at 14:14
  • 1
    \$\begingroup\$ Hmmm, that strikes me as unusual. IIRC the VBA interpreter which is responsible for running code, managing memory etc is hosted by Excel, so Excel has permission to close it whenever it fancies (like hitting the square Stop button) - well behaved VBA code that doesn't do weird stuff with WinAPI should never prevent Excel closing, regardless of circular references. This sounds more like an issue with maybe Add Ins unloading or a Workbook_BeforeClose event. As for debugging, I'd recommend sprinkling Debug.Print in the Class_Terminate handlers to check things are being closed as expected. \$\endgroup\$
    – Greedo
    Jan 11 '20 at 15:37
  • \$\begingroup\$ Awesome. Will follow your advice. Thanks! \$\endgroup\$ Jan 11 '20 at 20:55
6
\$\begingroup\$
If Not DebugMode Then Resume CleanExit Else: Stop: Resume

Avoid using the : instruction separator in code. That's good for golfing up quick throw-away code in the immediate pane, not for production code.

Statement syntax is fine when the Then part is a single statement - if it gets any more complex than that, block syntax should be used for clarity and maintainability (much easier to add statements in either conditional branch):

If Not DebugMode Then
    Resume CleanExit
Else
    Stop
    Resume
End If

It's not clear where DebugMode is defined, but it's clearly not a conditional compilation constant... which means the compiled code includes these debugging helpers. Not a big deal, but essentially the equivalent of shipping a release with the .pdb debug files.

Consider defining DebugMode as a conditional compilation constant, and then the condition can be conditionally compiled, resulting in this code with DebugMode=1:

    Stop
    Resume

...and this code with DebugMode=0:

    Resume CleanExit

While the source code would look like this:

#If DebugMode = 0 Then
    Resume CleanExit
#Else
    Stop
    Resume
#End If

That way no opcodes will be generated for the dead code when DebugMode is toggled on or off, and no condition needs to be evaluated at run-time; static code analysis (Rubberduck) will not see the dead code either, so StopKeywordInspection will only fire a result when DebugMode = 1, which can make a great heads-up that you're about to release code that includes instructions that were intended purely for debugging purposes.


Avoid noisy banner comments - especially if they're just there to eat up screen estate:

'
' Private Members
' ---------------
'

'
' Public Members
' --------------
'

'
' Private Methods
' ---------------
'

Group your members that way - and then the fact that private methods are private methods among other private methods will be self-evident; comments that plainly state the obvious, should be removed.

'@Version(1)
'@Folder("App")

Option Explicit
'@PredeclaredId

Consider grouping all module annotations together - either above or under Option Explicit... just not on both sides of it: the day [future] you (or a future maintainer) want(s) to add a @ModuleDescription annotation, if annotations are scattered everywhere then new annotations will just end up being added wherever they're added.

'@Folder("App")
'@PredeclaredId
Option Explicit

If annotations are always consistently above Option Explicit, then the message to the maintainer is clear: we want annotations above Option Explicit, and a maintainer unfamiliar with the code would know to put any new ones where they belong.

Note that @Version isn't a legal Rubberduck annotation, and very likely will never be one: version information (and copyright, authorship, license, diff history, etc.) does not belong in source code. It belongs in a source control repository. If your code isn't under source/version control, then what does a "version" mean anyway? I'd just remove it, it's a noisy comment that poses as a Rubberduck annotation, likely flagged by the IllegalAnnotation inspection.


ReplaceStringInWorkBook is iterating ThisWorkbook.Queries, which makes it very, very misleading. Since it's Private, I'm struggling to see what justifies @IgnoreProcedureNotUsed here - a private method with a misleading name that isn't invoked from anywhere, is dead code that needs to be removed.

A Public procedure (wait why is it in the middle of a bunch of Private methods?) in a framework-type project might be legitimately unused, but the AssignedByValParameter is a real concern here:

'@Ignore ProcedureNotUsed, AssignedByValParameter
Public Sub TransferQueries(Optional ByVal FromWorkbook As Workbook, Optional ByVal ToWorkbook As Workbook, Optional ByVal overwrite As Boolean = False)

By assigning to the supplied parameter, the rest of the procedure loses the ability to tell what the supplied values were. Whether or not the rest of the procedure needs that knowledge makes no difference: the Optional parameters are suspicious and make the public API ambiguous and adds implicit behavior to the calling code... and implicit behavior in an API, while pretty common in the Excel object model, should be avoided like the plague in any modern piece of code. If you don't want to declare and assign local variables instead, consider making the parameters non-optional, and raising an error if FromWorkbook or ToWorkbook isn't specified. If you really need a method that does this with ThisWorkbook, consider exposing a TransferQueriesFromThisWorkbook method that makes it explicit, doesn't take a FromWorkbook argument, and simply passes ThisWorkbook as the first argument to TransferQueries.

Note that ThisWorkbook is an identifier that refers to the host document's ThisWorkbook component, while Application.ThisWorkbook refers to the same, but then if you renamed ThisWorkbook to something else, the VBA project component would need to be updated, but Application.ThisWorkbook will always refer to the host document... except if you renamed ThisWorkbook, then Application.ThisWorkbook gets confusing - consider referring to the host workbook using the ThisWorkbook module identifier like you do for every single other module in your VBA project (and like you're doing everywhere else), because Application.ThisWorkbook is still going to be available through the [_Global] interface, which means renaming ThisWorkbook to SomeWorkbook will make [Application.]ThisWorkbook refer to SomeWorkbook, but as a member call against Application, won't get renamed by Rubberduck's rename refactoring.

Consider ditching the "Manager" suffix - see I Shall Call It... SomethingManager on Jeff Atwood's Coding Horror blog for more information.

\$\endgroup\$
9
  • 1
    \$\begingroup\$ I keep learning all the time. Thank you for these great reviews. Great reading about naming classes (in deed it's hard!). \$\endgroup\$ Jan 13 '20 at 18:41
  • \$\begingroup\$ What's your opinion on the Public Function AppWorkbook() As App? \$\endgroup\$ Jan 13 '20 at 19:12
  • \$\begingroup\$ @RicardoDiaz If it's meant to be invoked often (for every access to its properties and encapsulated objects), then it shouldn't be a Function but a Property Get that merely returns an instance that lives as long as the host workbook does -- and I'd put the setup code in the Workbook.Open handler. \$\endgroup\$ Jan 13 '20 at 19:26
  • 1
    \$\begingroup\$ If the code crashes with an unhandled error in production, you have a serious bug that you should be fixing instead of bending over backwards to keep the app running despite having serious issues :p I guess what I mean is, unhandled is unhandled, you don't know that the error is even recoverable - a hard crash is sometimes the best way to handle things. \$\endgroup\$ Jan 13 '20 at 20:04
  • 1
    \$\begingroup\$ As for Static locals to hold instance state, IMO instance state should live at instance level. Besides "static" in VBA means something very different than it does in every single other programming language, so should be avoided ...but maybe that's just me. \$\endgroup\$ Jan 13 '20 at 20:07

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.