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))))