Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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.

Source Link
Comintern
  • 4.2k
  • 1
  • 18
  • 26

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.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /