Names ----------- dirSeparator: I'd love to see this as something much shorter like dirSep. I think that dirSeparator is overly long - particularly for something which is likely to be used primarily in longer expressions.
currentDirSymbol and parentDirSymbol: The same here. These are _way_ longer than the strings that they represent. If we have them, they should be much shorter (e.g. currDirSym and parDirSym). But really, I think that I have to concur with dsimcha on this one. The strings for these are completely standard, easy to remember, and even shorter names for them would still be longer than the actual strings by a fair margin. We should just toss them. isDirSeparator: I'd love to see this shortened to isDirSep. extension: I'd love to see this shortened to ext. stripExtension: I'd love to see this shortened to stripExt. setExtensions: I think that it should have replace in its name rather than set, since it's not altering the original string. It's replacing what's there, not setting it. And again, I think that Extension should be shortened to Ext, so the renamed function would be replaceExt. defaultExtension: This function is not a property, so it probably shouldn't be a noun, though I'm not quite sure what to call it. Technically, it's something like setWithDefaultExtensionIfNoExtension, but that's obviously a horrible name even if you abbreviate some of it. So, I don't really have a better name, though if it's not gonig to be completely renamed, I definitely think that it should be shortened to defaultExt rather than having the full Extension in its name. joinPath: I'm not sure that this function should be renamed to joinPath. When discussing other modules such as as std.ascii and std.uni, we decided that they should have the same function names and _not_ put the module name in their names. Now, this function _is_ joining paths, so it's not purely a case of having the module's name in the function name, but I'm still not sure that we want to have it as joinPath instead of join. absolutePath: It really shouldn't be a noun, since it's not a property. Something like makeAbsolute would be better. Implementation ------------------------ You should probably consider testing the various functions with varying string types - both in size and mutability. Just put the test in a foreach loop like foreach (S; TypeTuple!(string, wstring, dstring)) (but with whatever combination of string types you want to test; In the ideal case it would be every possible combination of character type and mutability, but that's not necessarily necessary). Then you'll probably end up using to!S() on pretty much every string. And for functions which take multiple strings, you can test with a combination of string types. It'll make sure that the functions work appropriately with the various types. The functions are templated, but sometimes something gets missed and they don't work with a particular character size or level of mutability. So, while it might be overkill to test _every_ combination, verifying at least some of them would be desirable. Personally, I think that it's better practice to test for empty rather than length == 0, but I don't suppose that it really matters given that we're always dealing with arrays here. Your note on baseName about it adhering to the requirements for Posix's basename utility should probably put in the actual documentation. The same for dirName. There's no point in checking for "== null" like you do in a few places. If you want to check whether it's actually null, you need to check for "is null", and if you want to check whether it's empty, then you should just use empty. "== null" has a tendancy to be misleading, because people tend to think that it actually checks whether an array is null, which it doesn't do. Personally, I'm likely to think that it's a bug when I see it. It would probably be more efficient for setExtension to use ~=. e.g. auto retval = stripExtension(path); retval ~= '.'; retval ~= ext; return cast(typeof(return))(retval); Maybe it doesn't matter (and the code _is_ a bit less clean that way), but it might reduce the number of necessary heap allocations. The same goes for pretty much anywhere else that ~ is used. It probably doesn't matter all that much, but if we want to get the functions as efficient as possible, then it's better to use ~= than ~ unless the compiler is actually smart enough to tranlate the ~'s into ~='s, but I doubt that it is (particularly since some chunk of that is in druntime rather than in the compiler itself). There's no such thing as an OutOfMemoryException. It's an OutOfMemoryError. And being an error, I don't think that there's any need to mention it in the documentation. _Anything_ which allocates memory could cause the application to run out of memory, and being an Error, you're really not supposed to catch it, so there's no real point in mentioning it in the documentation. Okay. I think that that covers it, though I might have missed something. Aside from a few nitpicks, my main issue with the module as it stands is the unnecessarily long function names. The function names should be as long as is necessary to make them clear but not longer. And putting Extension and Separator in names is completely unnecessary. Ext and Sep are quite clear enough (especially in context) and are _much_ shorter. The longer names add nothing and make the functions/variables much more annoying to deal with in longer expressions. So overall, I think that it looks good and is a definite improvement, but I really think that the function names need to be shortened. - Jonathan M Davis
