How would someone more seasoned with elisp/functional programming write this function?

First, the problem statement. I wanted to traverse an output of the `libxml-parse-html-region` function, whose output takes the following form (where `tag` is a symbol, `attribute-list` is nil or an alist, and any remaining elements in the list are either strings or nodes following the same format): (tag attribute-list &rest child-nodes) I would then want to check if there are values in the attribute-list, and if there are to see if the (key . value) pair I'm looking for exists. If not, I would like to recursively pass through each child-node until I either find my (key . value) pair, or find nothing matching and return nil. &#x200B; For an example of the type of structure, this: <li>Genesis will remove all currently active <a href="/wiki/Curses" title="Curses">curses</a>, meaning Curse of the Blind will not affect the item pedestals present inside the room.</li> would become this: (li nil "Genesis will remove all currently active " (a ((href . "/wiki/Curses") (title . "Curses")) "curses") ", meaning Curse of the Blind will not affect the item pedestals present inside the room.") and a sample search would be to find the `<a/>` node with the attribute `'title` that has value `"Curses"`. &#x200B; Knowing this, I wrote the following function that takes the node to start with, the attribute name and value to find, and traverse the tree of child-nodes until I find and set a return value. (defun my/find-html-node-by-attr (node attr-name attr-value) (when (listp node) (let ((child-nodes (nthcdr 2 node)) (result)) (unless result (if (and (consp (nth 1 node)) (equal (cdr (assoc attr-name (nth 1 node))) attr-value)) (setq result node) (progn (dolist (child-node child-nodes result) (unless result (setq result (my/find-html-node-by-attr child-node attr-name attr-value))))))) result))) Sample call: `(my/find-html-node-by-attr parsed-html 'title "Curses")` This is obviously heavily inspired by a background in non-functional programming languages, and you can probably see exactly where I would have put "return" statements. The `setq`'s with the result value come from looping over the child-nodes with a dolist, and I know there has to be a better way but I can't quite get it. Just getting a function that worked took several hours, and led to several "breakthroughs" about what people talk about when they say lisps are great because code is data, but I just can't quite wrap my head around how this *should* be written. Having a more functional version of the same thing would be very helpful to help start bridging that gap, and I'm not really sure where else to post this.

(require 'cl-lib) (defun my/find-html-node-by-attr (node attr-name attr-value) (cl-destructuring-bind (tag attrs &rest childs) node (if (equal attr-value (alist-get attr-name attrs)) node (catch 'done (dolist (child (cl-remove-if-not #'consp childs)) (when-let ((found (my/find-html-node-by-attr child attr-name attr-value))) (throw 'done found))))))) (with-temp-buffer (save-excursion (insert "<li><li>Genesis will remove all currently active <a href=\"/wiki/Curses\" title=\"Curses\">curses</a>, meaning Curse of the Blind will not affect the item pedestals present inside the room.</li></li>")) (my/find-html-node-by-attr (libxml-parse-html-region (point) (point-max)) 'title "Curses")) Incidentally, answerers will never care what you tried. Bring on the downvotes.

Using the helpers from [dash.el](https://github.com/magnars/dash.el): (defun find-match (attr-val node) (seq-let (_ attrs &rest children) node (if (-any-p (-partial 'equal attr-val) attrs) ;; we're looking at it! node ;; search the list children, return the first non-nil value: (->> children (-filter 'listp) (-some (-partial 'find-match attr-val)))))) (defun parse-xml (xml-string) (with-temp-buffer (insert xml-string) (libxml-parse-xml-region (point-min) (point-max)))) (->> "<li>Genesis will remove all currently active <a href=\"/wiki/Curses\" title=\"Curses\">curses</a>, meaning Curse of the Blind will not affect the item pedestals present inside the room.</li>" (parse-xml) (find-match '(title . "Curses"))) ;; => (a ((href . "/wiki/Curses") (title . "Curses")) "curses") Thinking functionally takes time, good luck and feel free to ask any questions, thanks for the exercise

(defun my/find-html-node-by-attr (node attr-name attr-value) (when (listp node) (destructuring-bind (tag attrs . children) node (if (equal (cdr (assoc attr-name attrs)) attr-value) node (find-if #'my/find-html-node-by-attr children))))) Specific notes: + Why the first `unless result`? Result will always be nil there. + Why do you write `(setq result node)` instead of just `node`? If you just write `node`, then the return value of the if statement becomes `node` (unlike in most programming languages, the `if` in Lisp returns the value of whichever statement is executed. Similarly for `unless`). + Why do you use `progn` where you do? Progn is only useful when you need to put multiple statements where only one is usually allowed.

- This: (catch 'tag ;; to return a value early (throw 'tag value) ;; ... or (cl-block nil ;; ... (cl-return value) ;; ... are more idiomatic for writing code as if you have a return statement, as they actually jump out of the block. If you use cl-defun to define a function, the body is automatically placed in a block named after your function, so you can just use `(cl-return-from name-of-your-function value)`. (You can't just say `(cl-return value)` without a `(cl-block nil ...` though.) - All arguments to `if` other than the first two form the else block. This is sufficient: (if (my-predicate-p) (do-stuff) (message "This is the else block") (+ 1 2)) This is called an "implicit progn". - The result parameter to `dolist`: ;; Reversing a list should be done with `nreverse' as it is way faster ;; (it's a builtin function that has a dedicated opcode). ;; This is just for demonstration. (let (ret) (dolist (var (list 1 2 3) ret) (push var ret))) is simply short hand for (let (ret) (dolist (var (list 1 2 3)) (push var ret)) ret) . As you are discarding the value of that dolist statement, adding it there is just confusing. - Here's what I might do to the function: I'll first remove the useless progn and unless blocks: (defun my/find-html-node-by-attr/remove-useless (node attr-name attr-value) (when (listp node) (let ((child-nodes (nthcdr 2 node)) (result)) ;; `result' is always nil here as we've just initialized it to ;; nil above. Remove the useless `unless' check. (if (and (consp (nth 1 node)) (equal (cdr (assoc attr-name (nth 1 node))) attr-value)) (setq result node) ;; Remove extra progn (dolist (child-node child-nodes result) ;; If we reach the else block, the setq above has not been ;; executed. Remove the useless `unless'. (setq result (my/find-html-node-by-attr/remove-useless child-node attr-name attr-value)))) result))) The (setq result ...) is also mostly not necessary: (defun my/find-html-node-by-attr/last-value-return (node attr-name attr-value) (when (listp node) (let ((child-nodes (nthcdr 2 node))) (if (and (consp (nth 1 node)) (equal (cdr (assoc attr-name (nth 1 node))) attr-value)) ;; This is the last value if we go into this branch. So ;; this is the return value. node ;; dolist discards the return value, so we have to capture it. (let ((result)) (dolist (child-node child-nodes result) (setq result (my/find-html-node-by-attr/last-value-return child-node attr-name attr-value)))))))) The dolist will only keep the value from the last child node. Let's fix that: (defun my/find-html-node-by-attr/style-and-bug-fix (node attr-name attr-value) (when (listp node) ;; `cadr' = second element. (nth 1 node) is arguably more ;; readable; I just find `cadr' to be more succint.. (if (and (consp (cadr node)) (equal (cdr (assoc attr-name (cadr node))) attr-value)) node ;; Because `child-nodes' is only used in the else block, moving ;; the let binding here reduces the amount you have to scroll to ;; find the definition. (let ((child-nodes (nthcdr 2 node)) ;; This is another way to initialize a new local variable to nil. result) (dolist (child-node child-nodes) ;; This diverges from your original function, but I assume ;; you want *every* child, not just the last one. (push (my/find-html-node-by-attr/style-and-bug-fix child-node attr-name attr-value) result)) ;; `push' inserts in front, so (car result) is now the last ;; child node. Reverse it so first result is first child node. ;; ;; It is slower to insert from the end in the loop than to ;; insert in front than use `nreverse' to reverse it. This is ;; because `nreverse' is a builtin function, written in C, ;; which also has a dedicated bytecode. (nreverse result))))) Demo on using cl-block for early return. I'm not sure whether this is better than the previous one. (defun my/find-html-node-by-attr/early-return (node attr-name attr-value) (when (listp node) (cl-block nil (when (and (consp (cadr node)) (equal (cdr (assoc attr-name (cadr node))) attr-value)) (cl-return node)) (let ((child-nodes (nthcdr 2 node)) result) (dolist (child-node child-nodes) (push (my/find-html-node-by-attr/early-return child-node attr-name attr-value) result)) (nreverse result)))))

[deleted]

Use `seq-contains-p` to check whether this node has that attribute/value pair. If it doesn't, (ab?)use `seq-reduce` to return the child node that does: (defun my/find-html-node-by-attr (node attr-name attr-value) (when (listp node) (or (when (seq-contains-p (cadr node) (cons attr-name attr-value)) node) (seq-reduce (lambda (found child) (or found (my/find-html-node-by-attr child attr-name attr-value))) (cddr node) nil)))) (You may want to change the function signature to pass the attribute as `(name . value)`. it saves you code :)

I don't have dash.el installed, so I can't test it, but I think your function will return the node passed as parameter or a first level child only, since AIUI, `-first` doesn't return what the predicate returns. Can you test the case when it should return a more deeply nested node?

good catch, that should be a `-some` -- I initially had it as a -keep ->> first, and prematurely smooshed. updated op

I would start by seeing if one of the existing XML-querying libraries would do the job. https://alphapapa.github.io/emacs-package-dev-handbook/#outline-container-XML%20%2F%20HTML

Thank you for answering, that `destructuring-bind` function is especially nice, I felt like there had to be a way to do something similar but couldn’t find it. I’d be happy to answer your questions too. > Why the first  unless result ? Result will always be nil there. I put it in thinking it would prevent the function from running when called within itself if the result had been set already. Now that it’s been pointed out though it’s immediately clear it wouldn’t work that way. > Why do you write  (setq result node) This goes in hand with with the above comment, if result was set I wanted to return that instead of continuing to traverse the tree. That’s obviously not how that works though. > Why do you use  progn  where you do? I didn’t realize at the time that `if` would would evaluate everything remaining as the else branch, so the `progn` was to put the else case in a single statement.