elisp function to replace text in an org file

Question

I've made a little function for myself that replaces certain variables to text in an org file. I declare a bunch of variables in the beginning of the file and then the function replaces all references to the variable with the value. So for example

#+VAR:location=AAAAAA

text text text \v{location} text text text

becomes

text text text AAAAAA text text text

It's my first time really playing with elisp and I'm wondering if I used the language correctly and if there's possibility to improvement.

 (defun mp/org-fill-variables ()
 "Fill in the variables"
 (interactive)
 (let ((variable-count 0))
 (goto-char 0)
 (while (search-forward-regexp "^#\\+VAR:" nil t)
 (set-mark-command nil)
 (when (search-forward "=")
 (backward-char)
 (let ((variable (buffer-substring (region-beginning) (region-end))))
 (forward-char)
 (set-mark-command nil)
 (end-of-line)
 (let ((value (buffer-substring (region-beginning) (region-end))))
 ;; todo check if value non-nil
 (replace-string-in-region (concat "\\v{" variable "}") value)
 (setq variable-count (1+ variable-count))))))
 (message "replaced %d variables" variable-count)))

Answer 1

You can make a better (interactive) declaration. Because this modifies the buffer, we want to disable it on read-only buffers. We do that this way:

 (interactive "*")

Consider making the the function work within the active region, rather than the whole buffer:

 (defun mp/org-fill-variables (start end)
 "Fill in the variables"
 (interactive "*r")

search-forward-regexp is an alias for built-in function re-search-forward. I would prefer the latter as that's more familiar to other programmers. At first, I thought you had used one of the interactive functions.

We shouldn't be meddling with user's mark or position, so wrap the whole function in (save-excursion ), and don't use the interactive functions (forward-char) or (end-of-line).

(goto-char 0) is almost always wrong. Use (goto-char (point-min)) so that we do the Right Thing when narrowing is in effect.

Instead of (buffer-substring), we can get the variable name from the match results. That also allows us to be more specific about what a variable name looks like (e.g. no newlines):

 (while (re-search-forward "^#\\+VAR:\\(.+\\)=\\(.+\\)" nil t)
 (let ((name (match-string 1))
 (value (match-string 2)))

Consider counting the number of replacements as well as the number of variables found.

---

Modified code:

 (defun mp/org-fill-variables (start end)
 "Fill in the variables."
 (interactive "*r")
 (save-excursion
 (let ((variable-count 0)
 (replacement-count 0))
 (goto-char start)
 (while (re-search-forward "^#\\+VAR:\\(.+\\)=\\(.+\\)" nil t)
 (let* ((name (match-string 1))
 (value (match-string 2))
 (replaced (replace-string-in-region (concat "\\v{" name "}") value
 start end)))
 (setq variable-count (+ variable-count (if replaced 1 0))
 replacement-count (+ replacement-count (or replaced 0)))))
 (message "Replaced %d variable(s) in %d location(s)"
 variable-count replacement-count))))

• my_display_name: Thanks, this is very helpful and clear. I made the function like I would do when I do it myself as it started with a macro, hence the use of 'forward-char', 'search-forward-regexp', ...
• my_display_name: I had some time to test this, but it does not work (copypasted). If no region is active it says The mark is not active now and when I select the whole buffer it says End after end of buffer. How do I start debugging this?
• Toby Speight: Ah, that's the problem with (interactive "r") - it requires an active region. Perhaps make two functions, so that the buffer-wide one calls the region one with (point-min) (point-max) as arguments?
• Toby Speight: And the "End after end of buffer" error is a bug on my part - because we modify the buffer, that invalidates end. We need to (copy-marker end) early on and use that marker instead.
• my_display_name: Just to be sure. I added (end (copy-marker end)) in the let variable declaration list