Skip to main content
added 940 characters in body
Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 194
  • 468

The overall first impression is a very good one. Procedures are small, focused, generally well-named, everything is pretty much in its place - well done!

What follows is a series of observations, and suggestions / how I'd go about "fixing" them.


Controlling object lifetime

In my view, it's important to be able to reliably know whether any object pointer is valid at any given point in the code that needs to dereference that pointer: for any object we create and consume, we want to be able to control when it's created, and when it's destroyed.

So while this is procedural code and we're not going to fuss much about coupling, we can still flag the auto-instantiated object:

While convenient, an auto-instantiated,a global-scope auto-instantiated FSO object isn't something I'd recommend. Kudos for early-binding (side note: consider qualifying the library, e.g. As Scripting.FileSystemObject), but like anything accessing external resources (e.g. database connection, file handle, etc.), IMO its scope and lifetime should be as limited as possible. With a global-scope As New declaration, you give VBA the entire control over that object's actual lifetime.

MoreoverAlternatively, it doesn't even need to be declared if a With block is holdingcould hold the object reference in a tight, well-defined local scope, and we wouldn't even need to declare a variable for it:


Portability

VBA code that doesn't need to be tied to a particular specific VBA host application's object model library, should avoid such dependencies.

The Source parameter should be a VBProject object, not a Workbook; by taking in an Excel.Workbook dependency, you are needlessly coupling thisthe module tobecomes needlessly coupled with the Excel object model: if you needed to reuse this code in the future for, say, a Word VBA project, you'd need to make changes. IMO this SourceControlH (what does this H stand for anyway?) module should be as host-agnostic as possible, and work solely off the VBIDE Extensibility object model.


Consistency

Qualifying members is nice! Consistently qualifying members is better :)

Why is this Fso qualified with the module name, but not the one in ThisWorkbook? Consistency matters here: either globals are fully qualified (I'd recommend that), or they aren't - but sometimes qualified and other times not qualified is distracting; withoutWithout Rubberduck to help, a reader would need to navigate to the definition to make sure it's referring to the same object.


Other notes

I like your centralized approach to error-raising, but very much! I find the term "exception" a bit misleading though (if it's an exception, where's my stack trace?), and the procedure names read like properties. I'd propose something like this:

The Exception module being a class feels a bit wrong, even more so given the @PredeclaredId Rubberduck annotation, which presumably was synchronized and indicates the class has a VB_PredeclaredId = True attribute value: the class is never instantiated, only its default instance is ever invoked. The .NET equivalent is a static class with static methods, and the idiomatic VBA equivalent is a standard procedural module.

While convenient, an auto-instantiated, global-scope FSO object isn't something I'd recommend. Kudos for early-binding, but like anything accessing external resources (e.g. database connection, file handle, etc.), IMO its scope and lifetime should be as limited as possible.

Moreover, it doesn't even need to be declared if a With block is holding the object reference:

The Source parameter should be a VBProject object, not a Workbook; by taking in an Excel.Workbook dependency, you are needlessly coupling this module to the Excel object model: if you needed to reuse this code in the future for, say, a Word VBA project, you'd need to make changes. IMO this SourceControlH (what does this H stand for anyway?) module should be as host-agnostic as possible, and work solely off the VBIDE Extensibility object model.

Why is this Fso qualified with the module name, but not the one in ThisWorkbook? Consistency matters here: either globals are fully qualified (I'd recommend that), or they aren't - but sometimes qualified and other times not qualified is distracting; without Rubberduck to help, a reader would need to navigate to the definition to make sure it's referring to the same object.

I like your centralized approach to error-raising, but I find the term "exception" misleading (if it's an exception, where's my stack trace?), and the procedure names read like properties. I'd propose something like this:

The Exception module being a class feels wrong, even more so given the @PredeclaredId Rubberduck annotation, which presumably was synchronized and indicates the class has a VB_PredeclaredId = True attribute value: the class is never instantiated, only its default instance is ever invoked. The .NET equivalent is a static class with static methods, and the idiomatic VBA equivalent is a standard procedural module.

The overall first impression is a very good one. Procedures are small, focused, generally well-named, everything is pretty much in its place - well done!

What follows is a series of observations, and suggestions / how I'd go about "fixing" them.


Controlling object lifetime

In my view, it's important to be able to reliably know whether any object pointer is valid at any given point in the code that needs to dereference that pointer: for any object we create and consume, we want to be able to control when it's created, and when it's destroyed.

So while this is procedural code and we're not going to fuss much about coupling, we can still flag the auto-instantiated object:

While convenient, a global-scope auto-instantiated FSO object isn't something I'd recommend. Kudos for early-binding (side note: consider qualifying the library, e.g. As Scripting.FileSystemObject), but like anything accessing external resources (e.g. database connection, file handle, etc.), IMO its scope and lifetime should be as limited as possible. With a global-scope As New declaration, you give VBA the entire control over that object's actual lifetime.

Alternatively, a With block could hold the object reference in a tight, well-defined local scope, and we wouldn't even need to declare a variable for it:


Portability

VBA code that doesn't need to be tied to a particular specific VBA host application's object model library, should avoid such dependencies.

The Source parameter should be a VBProject object, not a Workbook; by taking in an Excel.Workbook dependency, the module becomes needlessly coupled with the Excel object model: if you needed to reuse this code in the future for, say, a Word VBA project, you'd need to make changes.


Consistency

Qualifying members is nice! Consistently qualifying members is better :)

Why is this Fso qualified with the module name, but not the one in ThisWorkbook? Without Rubberduck to help, a reader would need to navigate to the definition to make sure it's referring to the same object.


Other notes

I like your centralized approach to error-raising very much! I find the term "exception" a bit misleading though (if it's an exception, where's my stack trace?), and the procedure names read like properties. I'd propose something like this:

The Exception module being a class feels a bit wrong, even more so given the @PredeclaredId Rubberduck annotation, which presumably was synchronized and indicates the class has a VB_PredeclaredId = True attribute value: the class is never instantiated, only its default instance is ever invoked. The .NET equivalent is a static class with static methods, and the idiomatic VBA equivalent is a standard procedural module.

Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 194
  • 468

'@Ignore EncapsulatePublicField
Public Fso As New FileSystemObject

While convenient, an auto-instantiated, global-scope FSO object isn't something I'd recommend. Kudos for early-binding, but like anything accessing external resources (e.g. database connection, file handle, etc.), IMO its scope and lifetime should be as limited as possible.

Moreover, it doesn't even need to be declared if a With block is holding the object reference:

Private Sub Workbook_AfterSave(ByVal Success As Boolean)

    Dim ExportFolder As String
    ExportFolder = ThisWorkbook.Path & "\src"
    
    With New Scripting.FileSystemObject
        If Not .FolderExists(ExportFolder) Then
            .CreateFolder ExportFolder
        End If
    End With

    SourceControlH.ExportProjectComponents ThisWorkbook, ExportFolder

End Sub

Note that the {bool-expression} = False condition is redundant - comparing a Boolean expression to a Boolean literal is always redundant: Not {bool-expression} is more idiomatic, more concise, and more expressive.

Public Sub ExportProjectComponents(ByVal Source As Workbook, ByVal Path As String)

The Source parameter should be a VBProject object, not a Workbook; by taking in an Excel.Workbook dependency, you are needlessly coupling this module to the Excel object model: if you needed to reuse this code in the future for, say, a Word VBA project, you'd need to make changes. IMO this SourceControlH (what does this H stand for anyway?) module should be as host-agnostic as possible, and work solely off the VBIDE Extensibility object model.

If Tools.Fso.FolderExists(Path) = False Then

Why is this Fso qualified with the module name, but not the one in ThisWorkbook? Consistency matters here: either globals are fully qualified (I'd recommend that), or they aren't - but sometimes qualified and other times not qualified is distracting; without Rubberduck to help, a reader would need to navigate to the definition to make sure it's referring to the same object.

But, then again, I'd New up the FSO on the spot, and let VBA claim that pointer as soon as it's no longer needed:

With New Scripting.FileSystemObject
    If Not .FolderExists(Path) Then
        Exception.DirectoryNotFoundException "Path", ModuleName & "." & MethodName
    End If
End With

I like your centralized approach to error-raising, but I find the term "exception" misleading (if it's an exception, where's my stack trace?), and the procedure names read like properties. I'd propose something like this:

    Errors.OnDirectoryNotFound "Path", ModuleName & "." & MethodName

It removes the doubled-up "Exception" wording from the instruction, and the On prefix is reminiscent of the .NET convention to name event-raising methods with that prefix.

The Exception module being a class feels wrong, even more so given the @PredeclaredId Rubberduck annotation, which presumably was synchronized and indicates the class has a VB_PredeclaredId = True attribute value: the class is never instantiated, only its default instance is ever invoked. The .NET equivalent is a static class with static methods, and the idiomatic VBA equivalent is a standard procedural module.

Of course Public Sub procedures in a standard module would be visibly exposed as macros in Excel, and using a class module prevents that... but so does Option Private Module!

Side note, there's a spelling error in this message:

    Exception.InvalidOperationException "Source.VBProject.Protection", _
                                        "The VBA project, in this workbook is protected " & _
                                        "therefor, it is not possible to export the components. " & _
                                        "Unlock your VBA project and try again. " & ModuleName & "." & MethodName

The comma after The VBA project is superfluous, there should be a dot after is protected, and so therefor should be Therefore, capital-T.

That said, VBA project protection can easily be programmatically thwarted, so with a little tweaking I think you could make this macro a bad boy that can just unlock a locked project to export it - but yeah, prompting the user with "oops, it's locked, try again" is probably the more politically-correct way to go about handling project protection.

I'm not finding any uses for the LinesCount function, and it validating whether the stream is open strikes me as weird: raising this error would clearly only ever happen because of a bug, and should be a Debug.Assert check, if present at all.

If ArrayH.Exists(Component.Type, ExportableComponentsTypes) = False Then

That H again? I'm starting to think it just stands for Helper, which is a code smell in itself. Once more, this condition would read better as If Not ArrayH.Exists(...) Then, but I'd like to point out that these helper methods feel very much like what would be extension methods in .NET-land, and ArrayExt.Exists - or better, a fully spelled-out ArrayExtensions.Exists would raise fewer eyebrows. Kudos for avoiding the trap of just dumping all "helper" procedures and functions into some Helpers bag-of-whatever module.

' Path to the folder where components will be saved.
Private pExportFolderPath As String

' Indicates if empty components should be exported or not.
Private pExportEmptyComponents As Boolean

This very much Systems Hungarian p prefix is distracting: there's no Hungarian Notation anywhere in the code and it reads like a charm - yes, naming is hard. Yes, naming is even harder in a case-insensitive language like VBA (or VB.NET).

You could make a simple, private data structure to hold the configuration state, and with that you wouldn't need any prefixing scheme:

Private Type ConfigState
    ExportFolderPath As String
    WillExportEmptyComponents As Boolean
End Type

Private Configuration As ConfigState

Note that because Export is a noun in the String variable, but a verb in the Boolean one, a distinction is necessary IMO. Adding a Will prefix to the Boolean name clarifies everything I find. And now you can have properties named exactly after the ConfigState members, without any prefixing scheme - note the Rubberduck annotation opportunity for a @Description annotation, too:

'@Description("Indicates if empty components should be exported or not.")
Public Property Get WillExportEmptyComponents() As Boolean
    WillExportEmptyComponents = Configuration.WillExportEmptyComponents
End Property
    
Public Property Let WillExportEmptyComponents(ByVal Value As Boolean)
    Configuration.WillExportEmptyComponents = Value
End Property

Speaking of Rubberduck opportunities, the @Folder organization can be enhanced - using @Folder annotations on the project's wiki describes how the annotation can be used to create subfolders:

'@Folder("Parent.Child.SubChild")

We have SourceControlH and ArrayH modules under '@Folder("Helper"), some Tools module (FWIW "Tools" has the exact same smell as "Helper" does) under '@Folder("Lapis"); the Exception module is under '@Folder("Lapis") as well, which means the tree structure looks like this:

- [Helper]
   - ArrayH
   - SourceControlH
- [Lapis]
   - Tools
   - Exception

Not sure what Lapis means, but the contents of the Tools module has this "whatever couldn't neatly fit anywhere else" bag-of-whatever feeling to it. What I wonder though, is why there's no clear dedicated SourceControl folder.

I'm not going to claim a more OOP approach would even be warranted here (procedural is perfectly fine), but the basis for sticking to procedural feels wrong: it's not a self-contained module, it has dependencies and must be packaged as a "bunch of modules that need to be imported together" anyway.

Having a Helpers.SourceControl folder would give you the dedicated space to cleanly split responsibilities while keeping the components neatly regrouped (in Rubberduck's Code Explorer toolwindow, that is).

' Full name means - name of the component with an extension.
Dim FullName As String: FullName = GetComponentFullName(Component)

I've seen Microsoft claim using the : instruction separator like this is "good practice" and "helps transition to VB.NET syntax". I'm not buying it at all. It looks awful and crowded. That comment is also very informative: it reads "this GetComponentFullName procedure needs a better name". In the Excel object model, FullName includes not only the file extension, but also the full path: your version of "full" isn't quite as "full" as it should be. In fact, FullName is actually nothing more than a fileName:

Dim fileName As String
fileName = GetComponentFileName(Component)

Kudos here:

       .Add vbext_ct_Document, "doccls"

This file extension is compatible with Rubberduck's own handling of document modules. By default, the VBIDE API exports document modules with a .cls file extension, which makes them import as class modules: to import them back into a Worksheet module, or into ThisWorkbook, you need some special handling, and that different file extension works great.

Source-controlling VBA code is hard, because the code in a document module (e.g. Worksheet) can very well include references to objects that exist in the host document, like ListObject tables and whatnot - and these can't really be under source control (not without having the whole host document under source control too!). Worksheet layout can't be restored from source code, unless the worksheet layout is itself actually coded: this means a VBA project restored from source control can't really ever fully restore a project without the original host document anyway. So, kudos for tackling this thorny issue.

Note that the last few pre-release builds of Rubberduck include bulk import/export functionality that does everything your code does, out of the box, without requiring programmatic access to the VBIDE Extensibility library, and without needing to share and manage versions for a SourceControlH module across devs and projects:

Rubberduck Code Explorer's "Sync Project" features