You can probably just leave the VBScript_RegExp_55 off the declaration of RegExp - pedantic mode was on solely to avoid the appearance of hypocracy appearance of hypocracy.
You can probably just leave the VBScript_RegExp_55 off the declaration of RegExp - pedantic mode was on solely to avoid the appearance of hypocracy.
You can probably just leave the VBScript_RegExp_55 off the declaration of RegExp - pedantic mode was on solely to avoid the appearance of hypocracy.
A couple things I noticed - all of your public methods rely on this.StringsToMatch
being initialized. You're using Is Nothing
as basically a proxy for .Count = 0
. Since the behavior of the class depends on having an initialized container, just create it once in Class_Initialize
:
Private Sub Class_Initialize()
Set this.StringsToMatch = New Scripting.Dictionary
End Sub
Then you can replace ClearTargetSubstrings
with the much more natural...
Public Sub ClearTargetSubstrings()
this.StringsToMatch.RemoveAll
End Sub
...and .StringsToMatch Is Nothing
with the much clearer .StringsToMatch.Count = 0
.
Along those same lines, I've always disliked the Scripting.Dictionary
behavior of adding an item as a side-effect of calling .Item(foo) = bar
if foo
doesn't exist. In my mind this should be an error, so it looks like an error at first glance. Just because Microsoft made a bad implementation decision doesn't mean you have to rely on their poor implementation at the expense of your readability. If you always have a .StringsToMatch
hanging around, you can make AddTargetSubstring
a lot clearer as to what it is actually doing:
Public Sub AddTargetSubstring(ByVal inValue As String)
With this.StringsToMatch
If Not .Exists(inValue) Then
.Add inValue, vbNull
End If
End With
End Sub
Note that this also allows you to add an additional dereference level to your With
block.
Note above that I set the key, but left the item as vbNull
. If the key and the item are always the same, there isn't any reason to store 2 copies of it. You are basically using the Scripting.Dictionary
as a simple hashset, so just use the part .Keys()
that actually is the hashset. Save the memory and hassle of actually storing copies as the .Item
.
I'll diverge a bit from @Mat'sMug's recommendation to have errors bubble up to the calling code. The class is simple enough that I wouldn't even be raising errors about its state. If you require that this.RootFolder
is set, make the property read-only and make a Scripting.Folder
a required parameter of SearchFolderForMatch
instead. If you don't have any sub-strings to search, pick a default behavior (like returning vbNullString
for MatchedFilePath
or matching any file) and do that instead of raising an error. That allows the calling code much more flexibility and removes the requirement for the caller to have to establish a valid object state before using certain (in this case 1) methods.
Short nit-pick break (and this is entirely a personal style preference) - I find the "out-dented" GoTo
labels distracting. The vast, vast majority of code that I see has them simply pinned to the left margin where the VBE puts them. Having them at a different indentation level makes think that they are acting as a closure, so it actually takes a bit of mental effort for me when I see this:
EndRecursion:
End Sub
Public Function NameContainsAnyTargetSubstring(ByVal nameToCheck As String)
'...
EndCheck:
The fact that they all start with End
makes it even worse. My brain does something like End Sub
, End Function
, EndCheck
, ...WTF?
Finally, you have a pretty decent performance opportunity in NameContainsAnyTargetSubstring
. You are comparing every single file to the same set of keys, so I'd consider converting it to just build a RegExp pattern and using that instead. Just getting rid of files * substrings
extraneous string casts with the line stringToFind = CStr(key)
is worth it alone.
I'd do something more like this. Make NameContainsAnyTargetSubstring
a simple wrapper function (mainly to avoid having to recreate the regex pattern repeatedly):
'Class level variable.
'Requires reference to Microsoft VBScript Regular Expressions 5.5
Private substringRegex As VBScript_RegExp_55.RegExp
Private Sub Class_Initialize()
Set this.StringsToMatch = New Scripting.Dictionary
Set substringRegex = New VBScript_RegExp_55.RegExp
substringRegex.IgnoreCase = True
End Sub
...and down below:
Private Sub SetRegexPattern()
substringRegex.Pattern = "(" & Join(this.StringsToMatch.Keys, ")|(") & ")"
End Sub
Public Function NameContainsAnyTargetSubstring(ByVal nameToCheck As String) As Boolean
If this.StringsToMatch.Count = 0 Then Exit Function 'Or whatever default.
SetRegexPattern
NameContainsAnyTargetSubstring = substringRegex.Test(nameToCheck)
End Function
Private Sub RecurseFolderForMatch(ByRef folderToRecurse As Folder)
SetRegexPattern
Dim candidate As File
For Each candidate In folderToRecurse.Files
With candidate
this.MatchFound = substringRegex.Test(.Name)
If this.MatchFound Then
this.MatchedFilePath = .Path
Exit Sub
End If
End With
Next
Dim subFolder As Folder
For Each subFolder In folderToRecurse.SubFolders
RecurseFolderForMatch subFolder
If this.MatchFound = True Then
Exit Sub
End If
Next
End Sub
You can probably just leave the VBScript_RegExp_55 off the declaration of RegExp - pedantic mode was on solely to avoid the appearance of hypocracy.