chore: refactor recursive search for markdown files#352
Conversation
thompson-tomo
requested review from
AndrewSouthpaw and
PeterDaveHello
as code owners
| function findRec(paths, syntax) { | ||
| var allowedExts = extensions[syntax] || []; |
There was a problem hiding this comment.
let's making a copy of the paths to avoid mutating the param, slightly less spooky
There was a problem hiding this comment.
Aren't we already making a copy of by adding the param into an array on https://github.com/thlorenz/doctoc/pull/352/files#L50 which is the only way to enter this function?
There was a problem hiding this comment.
Yes, and technically right now it's fine, but it isn't obvious. Mutating a param is quite often an unexpected behavior. There's only you and me working on this codebase, so it's kinda moot, but someone who didn't know any better and didn't look at the contents of a function might use findRec and reasonably think paths doesn't get affected. It's a general clean coding practice, and in this case I don't see a compelling argument for a perf win to avoid defensive copying.
There was a problem hiding this comment.
Ok, I think I have changed it as suggested.
|
@AndrewSouthpaw this is ready for another look. |
2 participants
Eliminate the statSync to optimise performance as that is often a costly operation which we do not need to be calling.
Overall optimisation of the get recursive ensuring streamlined operations and reduction in variable usage/allocations.