Problem
I have a set of directories. For each directory I have a set of relative file names. I would like to select any of these relative filenames using completion, and then later reconstruct the absolute path of the filename.
In order to create a list of relative filenames for a given directory, I created this function:
(defun my-get-dir-files (dir pat &optional rel-dir)
"Finds relative filenames. Returns a list of relative filenames/pathnames to
directory `dir' matching pattern `pat'. The variable `rel-dir` is only used internally
by recursive calls and should usually not be supplied.
IMPORTANT: This function uses recursion. To avoid excessive copying of lists,
the variable `files' is used as a dynamic list variable, and is assumed to be
defined as non-lexical dynamic variable in the caller using a let binding"
(let ((dir-files nil)
(cur-dir nil))
(unless rel-dir (setq rel-dir ""))
(setq cur-dir (expand-file-name rel-dir dir))
(setq dir-files (directory-files cur-dir nil nil t))
(dolist (name dir-files)
(unless (or (string= name ".") (string= name ".."))
(let ((fn (expand-file-name name cur-dir)))
(if (file-directory-p fn)
(let ((new-rel-dir nil))
(if (string= "" rel-dir)
(setq new-rel-dir name)
(setq new-rel-dir (concat rel-dir "/" name)))
(my-get-dir-files dir pat new-rel-dir)) ;; recursive call
(when (string-match pat name)
(let ((rel-fn nil))
(if (string= "" rel-dir)
(setq rel-fn name)
(setq rel-fn (concat rel-dir "/" name))
(setq files (append files `(,rel-fn))))))))))))
Then I plan to use code like:
(let ((dir "/home/hakon/dir1")
(files nil))
(my-get-dir-files dir "\.el$")
files)
to set the list variable files.
One thing that worries me here is the use of the (non-lexical) dynamic variable files. Can or should this be avoided?
According to the manual I have violated this principle:
If a variable has no global definition, use it as a local variable
only within a binding construct
Solution
Yeah don’t use a dynamic variable for that, it’s just confusing for the
user.
It is way easier to deal with input and output from a
function instead of having to take special variables into account, basically it’s now part of your interface. And I personally
would be really angry if a random function somewhere took a common
variable like files from outside its scope and filled it with data.
So, after that explanation, I would recommend using either a separate
function, or cl-flet/cl-labels to handle the recursive part and only
expose the necessary parts to the caller. That means no optional
rel-dir, which isn’t really part of the interface and no using files
from the outer scope.
To wit:
(defun my-get-dir-files (dir pat)
"..."
(let (files)
(cl-flet ((aux (dir pat rel-dir)
(let ((dir-files nil)
(cur-dir nil))
(unless rel-dir (setq rel-dir ""))
(setq cur-dir (expand-file-name rel-dir dir))
(setq dir-files (directory-files cur-dir nil nil t))
(dolist (name dir-files)
(unless (or (string= name ".") (string= name ".."))
(let ((fn (expand-file-name name cur-dir)))
(if (file-directory-p fn)
(let ((new-rel-dir nil))
(if (string= "" rel-dir)
(setq new-rel-dir name)
(setq new-rel-dir (concat rel-dir "/" name)))
(aux dir pat new-rel-dir)) ;; recursive call
(when (string-match pat name)
(let ((rel-fn nil))
(if (string= "" rel-dir)
(setq rel-fn name)
(setq rel-fn (concat rel-dir "/" name))
(setq files (append files `(,rel-fn)))))))))))))
(aux dir pat nil)
files)))
Note that I just wrapped the existing function one time and put a let
around it to keep the side effect more contained. And now that files
is lexical, everything is fine.
But really, it would be better to just use an accumulator. You
only ever add one element to the list, in which case you should just
immediately use push instead (and if you care about the output order,
nreverse the final result).
The following changes could also be made later:
- Use
memberinstead of theor, less typing for you. - (Short variable names don’t necessarily improve readability. Even
though Elisp code seems to disagree in places.) - The explicit binding to
nilisn’t necessary as it’s the default
anyway. - Also the
setqs there are unnecessary, just put the initial binding
into theletinstead, useorand other functional tools. condis more readable for nested conditions, if applicable.- I would use
listfor simple list constructions like (,foo). (setq rel-fn name)in theifdoesn’t do anything as it’s the only form in the true branch.
So with all that it would be like this (even though personally I’d
rename the fn to filename as well. YMMV:
(defun my-get-dir-files (dir pat)
(let (files)
(cl-flet ((aux (dir path rel-dir)
(let ((cur-dir (expand-file-name rel-dir dir)))
(dolist (name (directory-files cur-dir nil nil t))
(unless (member name '("." ".."))
(let ((fn (expand-file-name name cur-dir)))
(cond
((file-directory-p fn)
(aux dir pat (if (string= "" rel-dir)
name
(concat rel-dir "/" name))))
((string-match pat name)
(unless (string= "" rel-dir)
(push (concat rel-dir "/" name) files))))))))))
(aux dir pat "")
files)))
I’m pretty sure all transformations kept the meaning the same; a quick
check via
(set-exclusive-or (let (files) (my-get-dir-files-1 ".." ".*") files) (my-get-dir-files ".." ".*") :test #'equal)
with -1 being the original function resulted in no difference in the
lists (except for order of course), so that was enough for me. If you
return (nreverse files) instead the lists are exactly equal.
And at this point I’m done. There might still be potential
optimisations during the concatenations and so on. The recursion itself here is fine as you won’t encounter directory trees deep enough to cause the recursion to fail and it’s also fairly efficient now, so no need to change anything there.
You could still extract a local function for (if (string= "" rel-dir) ...) and use that there and in the other branch of the cond.
Next up: Would be cool if the function also handled "." for dir.