Naming, naming, naming.
I've never heard of a "glob search", but I like the idea of implementing it in vba. One thing I know, is that I don't expect vba to read like python, so os.SEP
would be clearer as os.PathSeparator
, or even better, as path.Separator
. Now I do have a tendency to go by .net naming standards, but really, anything other than a YELLCASE SEP
would be cleaner and clearer.
Even the python community seems to agree that using os.path.sep
over os.sep
makes for clearer code - I'd use os.path.sep
to make it very clear that it's the path separator and I recommend you use os.path.sep
for clarity, since it's a path separator, not an OS separator. So I'd go with path.Separator
, or path.Sep
to maintain Pythonish naming.
Looking at your linked question, I see @RubberDuck already recommended using better names for your constants; sorry to reiterate the same advice here.
Underscores
I know python loves snake_case
, but in vba that kind of casing is potentially confusing (for class methods, more specifically) - I'd recommend sticking to language's PascalCase
naming convention, and using a consistent camelCase
for locals and parameters, for readability's sake (the actual VB/VBA conventions would go PascalCase
all the way, but I find that maddening). I find pat_list
would really benefit from simply being called patterns
, or more accurately, patternParts
- the _list
suffix feels Hungarian, and seems to reflect the type, which could just as well be a regular Collection
- I like that you're using your own List
type though.
Parameters
The documentation I've found actually calls its sole parameter pathname
:
glob.glob(pathname)
Return a possibly-empty list of path names that match pathname, which must be a string containing a path specification. pathname can be either absolute (like /usr/src/Python-1.5/Makefile
) or relative (like ../../Tools/*/*.gif
), and can contain shell-style wildcards.
You haven't shown us how the client code might use this function, and it's.. pretty hard to guess just by looking at this post, without diving into the os
implementation - I'd have to jump to the definition of os.SubItems
to figure out why I need to pass a root
parameter on top of a pattern
.
Looking at your added example call, I think it could be worth combining the two parameters into a single pathName
parameter, making the function callable like this:
?Glob("C:\users\ptwales\????-herp\*\*.txt").ToString
The difficulty would be to parse the string and differenciate the root
from the pattern
- splitting on path separator and using anything to the left of a part that contains any wildcard as the root
, and everything else as the pattern
, would certainly be possible, and would make it a better API IMO; the root
and pattern
parts are really two parts of the same string, that you've split in two because it's more convenient to do so since the implementation calls os.SubItems
which wants a root
parameter - in other words, splitting that string in two is really only leaking implementation details into the API.
Function as a local variable
I don't like this:
Set GlobRecurse = New List
...
GlobRecurse.Extend GlobRecurse(folder, pat_list, index + 1)
While this certainly works, it is rather ugly. Not because of multiple assignments (roughly requivalent to multiple [non-returning] return
statements), but because the function's name is being used a local List
variable, happily being appended to.
I would much rather see this:
Private Function GlobRecurse(ByVal root As String, ByVal pat_list As List, ByVal index As Integer) As List
Dim result As List
If index = pat_list.Count Then
Set result = os.SubItems(root, pat_list(index))
Else
Set result = New List
Dim folder As Variant
For Each folder In os.SubFolders(root, pat_list(index))
result.Extend GlobRecurse(folder, pat_list, index + 1)
Next folder
End If
Set GlobRecurse = result
End Function
Especially with the recursive nature of the function, you'll want to avoid repeating the function's name for no reason. My rule of thumb, is to never read from the function identifier, only to assign it.
Glob("C:\path\to\file\*\*.txt")
? \$\endgroup\$