14
\$\begingroup\$

Starting point I kind-of inherited a VBA project at work that consists of a bunch of Word document templates that provide users with the tools they need to create documents according to company guidelines and to help them give all documents the same appearance. We use different templates for the documents that we create, and each of these uses a different subset of tools from the collection. I was faced with the problem of having to maintain multiple copies of the same code in as many different templates.

Current Solution The solution I came up with was to group associated code fragments into separate modules where each serves a well-defined purpose, to export these modules to external files and to build a preloading process that is triggered from AutoNew() and AutoOpen(). One advantage of this is that I can change the code in these modules without having to wait for write access to the templates that need them.

Failed Ideas What I tried but failed at is to move the code to another template and to link that to the document template via the VBE in an attempt at faking some kind of inheritance. This approach consistently led to crashes of the macro system as well as (occasionally) Word itself. What's more, the error codes that Word decided to spew at me were entirely unhelpful, which is the main reason why I threw all of this in the bin - I didn't feel equipped to even start debugging it.

What's probably not going to work What I would like to avoid, though, is to write a Word AddIn or using VSTO because we do not have any meaningful way of deploying new versions of the code to the users' workstations, and I'd hate the prospect of having to tamper with their machines every time I fix something. For my personal convenience, I'd pretty much prefer if everything could just sit in our group's template directory on our central network share.

Well, here's the monstrosity I created and that I'd like your opinion on:

' Constants are public because they are defined in a different module and I 
' just copied them here for the purpose of this post.
' path to the data storage directory
Public Const DATADIR_PATH As String = "K:\msoffice\data\"
' path to the code modules
Public Const MODULE_PATH As String = DATADIR_PATH & "modules\"
' list of additional modules that this template requires
Public Const IMPORT_MODULES As String = "table_tools.bas,ParseHeader.bas,drawing_tools.bas"

Sub AutoOpen()
  ' Only preload code when the opened document is NO template because
  ' that would lead to the modules being saved with the template, which
  ' breaks the concept.
  If (ActiveDocument.Type <> wdTypeTemplate) Then
    PreloadAdditionalModules
  End If
End Sub

Sub AutoNew()
  PreloadAdditionalModules
End Sub

' Load additional code modules into the project
Private Sub PreloadAdditionalModules()
  Dim Module As Variant
  Dim ModulesToLoad() As String
  ' Split() is the only way to ensure that the array exclusively consists
  ' of strings.
  ModulesToLoad = Split(IMPORT_MODULES, ",")

  ' Load all listed modules into the project that do not yet exist.
  For Each Module In ModulesToLoad
    If (Dir(MODULE_PATH & Module) = "") Then
      MsgBox "Critical error: Unable to load module " & Module & ". "& _
             "Network share unreachable?", vbExclamation
      ActiveDocument.Close False
      Exit Sub
    ElseIf (ModuleExists(Module) = False) Then
      ThisDocument.VBProject.VBComponents.import FileName:=MODULE_PATH & Module
    End If
  Next Module
  ' Loading a module into the project constitutes a change of the template file,
  ' therefore prompting Word to ask the user if it should save the changes to disk.
  ' This would break the concept by permanently embedding the "dynamically" 
  ' loaded modules into the template. Therefore, we mark the template as "already saved."
  ThisDocument.Saved = True
End Sub

Private Function ModuleExists(ByVal WantedModule As String) As Boolean
  Dim count As Integer

  ModuleExists = False
  ' Module filenames are passed including their filename extensions. As the module
  ' names are identical to their file names, the extension hinders comparison with
  ' existion module names. It is therefore cut off beforehand.
  WantedModule = Split(WantedModule, ".")(0)
  With ThisDocument.VBProject.VBComponents
    For count = 1 To .count
      If (.Item(count).name = WantedModule) Then
        ModuleExists = True
        Exit For
      End If
    Next count
  End With
End Function

P.S.: Curiously, Rubberduck-VBA 1.4.3.2343 throws a "mismatched input" error on the expression Split(WantedModule, ".")(0).

However, because this also requires every workstation to allow all access to the VBA object model, I feel like this approach poses an inherent security risk, and I would very much like to avoid it but can't.

Thank you very much for taking the time to read all this, and for any input you may have.

\$\endgroup\$
7
  • 6
    \$\begingroup\$ Welcome to Code Review. Always nice to see a new VBA person, especially one who already has Rubberduck. \$\endgroup\$ – Kaz Apr 21 '16 at 12:52
  • 1
    \$\begingroup\$ Sidenote: the "mismatched input" error is a known and fixed grammar bug with methods returning arrays. Until the release of 2.0 in a few weeks a workaround is to save the result of the method call into a variable and access the 0-th element of that variable \$\endgroup\$ – Vogel612 Apr 21 '16 at 12:53
  • \$\begingroup\$ Additional sidenote: The Rubberduck team (and a lot of the CR VBA community) can be found here and the main site chatroom can be found here. Feel free to come say hi. \$\endgroup\$ – Kaz Apr 21 '16 at 12:59
  • \$\begingroup\$ If you edit the template, but the user already has the modules, how would it know to update? \$\endgroup\$ – Raystafarian Apr 21 '16 at 13:55
  • \$\begingroup\$ The update takes place the next time a user creates a new document based on one of the affected templates, i.e. "on startup". The code does not modify itself at runtime. \$\endgroup\$ – Chris Apr 21 '16 at 14:13
7
\$\begingroup\$

I have two tiny things right now. The variable count is a system reserved name for something else, so I would avoid naming the variable that way. countComponents might be good.

ElseIf (ModuleExists(Module) = False) Then

This logic can be simplified by editing the ModuleExists function to return the opposite.

ElseIf NoModuleExists(Module) Then

A simple adjustment here

Private Function NoModuleExists(ByVal WantedModule As String) As Boolean
  Dim countModules As Integer

  NoModuleExists = True

  WantedModule = Split(WantedModule, ".")(0)
  With ThisDocument.VBProject.VBComponents
    For countModules = 1 To .count
      If (.Item(count).Name = WantedModule) Then
        ModuleExists = False
        Exit For
      End If
    Next countModules
  End With
End Function

That would just increase the intuitive logic. Alternatively you could just use

ElseIf Not ModuleExists(Module) Then

But that is also using a False basis when a True is always more intuitive with if.


Speaking of checking for the module, you could simplify all that nesting by using the module objects as the loop.

Private Function NoModuleExists(ByVal WantedModule As String) As Boolean
  Dim targetModule As Object
  NoModuleExists = True

  For Each targetModule In Application.VBE.ActiveVBProject.VBComponents
    If targetModule.Name = WantedModule Then
        NoModuleExists = False
        Exit Function
    End If
  Next
End Function

Oh, one more thing on that integer - integers are obsolete. According to msdn VBA silently converts all integers to long.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your time and comments! I will definitely consider changing the logic of the ModuleExists() and also the loop thing inside it. Additionally, if 'count' is a reserved keyword, I'd have really loved the VBE to have said so when I used it. And about that integer thing - where I'm coming from, integer is the only type available in this regard, so its use is more about habits than anything else. Same applies to my putting every if condition in parentheses. \$\endgroup\$ – Chris Apr 21 '16 at 14:20
  • \$\begingroup\$ Yeah the VBE doesn't fail to "compile" or anything when using keywords, but I think strange things can happen. I never declare a range as Cell - but I don't think it's ever caused a problem. I hear you on the old habits die hard! \$\endgroup\$ – Raystafarian Apr 21 '16 at 15:09
7
\$\begingroup\$

Thanks for using Rubberduck! As @Vogel612 mentioned, the parser error you're getting is a known issue in the v1.4.3 release. In short:

foo = bar(x)(y)

Accessing a subscript immediately after a function call isn't supported. An easy work-around is to introduce a local variable to hold the collection/array result:

Dim result
result = bar(x)
foo = result(y)

Note that your entire code parses perfectly fine as-is with the current 2.0 build, which we're going to pre-release in a couple of weeks days.


The sad part is that, since Rubberduck couldn't parse your project, you couldn't benefit from the code inspections.. Not that there's a ton of results, but here's what 2.0 says:

  • Member AutoOpen is implicitly public
  • Member AutoNew is implicitly public

Module members are public by default, which can be counter-intuitive. Consider specifying explicit access modifiers to avoid ambiguity.

  • Parameter WantedModule is passed ByVal and assigned a value

Parameter is passed by value, but is assigned a new value/reference. Consider making a local copy instead if the caller isn't supposed to know the new value. If the caller should see the new value, the parameter should be passed ByRef instead, and you have a bug.

There are other results, but they're probably not relevant to your actual project (I just pasted your code into a new module, in Excel VBA) - Watch out for Option Explicit not being specified though.

Rubberduck inspection results toolwindow


Your indentation is well done and consistent, but you'll have to configure Rubberduck 2.0 indenter settings to make 2-space indent happen - default being 4 spaces.


Public Const DATADIR_PATH As String = "K:\msoffice\data\"

If K: is a network drive, know that it could be mapped (or re-mapped) to any other letter on other machines - it's probably a better idea to use the UNC path, like:

Public Const DATADIR_PATH As String = "\\servername\folder\msoffice\data\"

That way the path is accessible if you're on the network, regardless of whether your network drives are connected or not.


As the module names are identical to their file names

It's usually the case. However a module's actual name isn't determined by its filename, but by the value of a specific dedicated attribute in the file header (the VBE hides class headers and Attribute meta-instructions):

Attribute VB_Name = "table_tools"

In the unlikely event where someone would modify that attribute in, say, Notepad, your code would fail to identify an existing module, re-import a set of existing functions and procedures, and then the code wouldn't compile anymore because of duplicate definitions.

I'm not suggesting to stop assuming that the file name matches the module name, merely saying that the fact that the module name matches the file name is more of a convenient coincidence - and it's good to know where VBA actually gets the module name from, too =)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Thanks! I will certainly have a look into that module naming thing you mention because I'd really like to make my code more foolproof (at least until, you know, nature comes up with better fools). Regarding drive letter K:, I think I will continue to rely on that because this is a corporate environment and login scripts via domain controller ensure that every user gets their appropriate network drives. Additionally, the template running the code shown above also resides on K:, so access is pretty much granted. (Yes, I also rely on users not accessing the UNC directly.) \$\endgroup\$ – Chris Apr 21 '16 at 15:51
1
\$\begingroup\$

I don't have much to add, other than I noticed a complete lack of error handling. I'm going to guess that this template won't work correctly if the modules aren't loaded correctly. You should take some time to identify possible failure points and handle possible runtime errors by notifying the user that there was an issue loading required files, logging relevant info off someplace, and then shutting down the template.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your input, @RubberDuck. While it's correct that I don't use the usual error handling mechanisms like On error goto (mainly because I currently still lack the proficiency to do so effectively), I do check with Dir() if the files exist that I want the mechanism to load, notify the user if any of it fails, and shut down the template just as you suggest. You're absolutely right that everything blows up if this 'preloading' fails. - On a side note: I'd be very interested in your thoughts regarding possible security issues of my approach or alternatives to self-modifying projects. \$\endgroup\$ – Chris Apr 22 '16 at 14:00

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.