Refactoring a Simple Example of Simultanously User-Friendly and Debuggable Code

(defvar my-hs-hide nil "Current state of hideshow for toggling all.")
;; ###autoload
(defun my-toggle-hideshow-all () "Toggle hideshow all."
       (interactive)
       (setq my-hs-hide (not my-hs-hide))
       (if my-hs-hide
           (hs-hide-all)
         (hs-show-all)))

It works perfectly.

So does version 9.

It does precisely the same, plus a few warnings and simplifications. “Simplifications?!” surely you must be asking yourself, and the answer is yes—these are simplifications. Few would be shocked to see any of them, and many would find it easier to maintain. The most important thing is that the user has a better experience. There are few reasons to drop into a debugger unless something has gone very wrong. Granted, it happens a lot, but anyway lol, here you go. (Perhaps there are ways to simplify this more?)

Not to joke around too much, but when you write code like this, you are inviting people to be your friends when they take it over. It isn’t perfect, but it is a fine start to making the next maintainer’s and user’s day a little easier… I hope… I hope!

(defvar gcr-hs-visible 'all-visible "Current state of hideshow for toggling all.

If `\'all-visible' then every block is visible, otherwise it is `\'all-hidden', and they are all hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

  SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (condition-case-unless-debug err
      (progn
        (unless (and (boundp 'hs-minor-mode) hs-minor-mode)
          (user-error
           (concat "Hideshow either isn't loaded or enabled. "
                   "First load then enable it and try again."))
          (cl:return))
        (pcase (symbol-value 'gcr-hs-visible)
          ('all-visible
           (setq gcr-hs-visible 'all-hidden)
           (hs-hide-all)
           (message "Hiding all code blocks"))
          ('all-hidden
           (setq gcr-hs-visible 'all-visible)
           (hs-show-all)
           (message "Showing all code blocks"))
          (_
           (error
            (concat "(gcr-hs-visible) Sorry, I couldn't handle toggling "
                    "`%s'. Please notify me.")
            gcr-hs-visible))))
    (error
     (message
      "(gcr-hs-visible) Sorry, an error occurred: %s" (error-message-string err)))))

Read on for version 2-9: the details.

When I bring in code to my config I usually perform some default integration tasks like this:

1. Remove the autoload: It isn’t necessary
2. Attribution: So I can remember
3. Formatting: Put the doctring in a newline
4. Namespacing: Prepend it with gcr

(defvar gcr-hs-hide nil "Current state of hideshow for toggling all.")
(defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (build in) `hideshow.el.gz'"
       (interactive)
       (setq gcr-hs-hide (not gcr-hs-hide))
       (if gcr-hs-hide
           (hs-hide-all)
         (hs-show-all)))

After staring at this, I noticed a code pattern I don’t prefer: using a double-negative for status checking. That got my attention. It’s not a big deal; it’s just a personal preference since now this code “is my problem,” so to speak (unconsciously, admittedly, I might be looking for an excuse to code). Additionally, it is easier to understand when code is visible as t or hidden as nil (not) visible. So I stared at it a little more and came up with this:

1. Invert the state logic: Non-nil if visible, nil if hidden
2. Add more documentation: So the user understands how to use it

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (build in) `hideshow.el.gz'"
       (interactive)
       (setq gcr-hs-visible (not gcr-hs-visible))
       (if gcr-hs-visible
           (hs-show-all)
         (hs-hide-all)))

Because I’ve stared at the the door is now wide-open to refactor even more.

1. From if to cond: if statements are ugly. Additionally they make it mildly irritating to add a third state which we always to at some point.
2. Move the toggle into the cond: not even being tricky to save a line, it is just easier to to read.
3. cl-defun: Switch it over just in case, and there is no speed penalty

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (cond ((setq gcr-hs-visible (not gcr-hs-visible)) (hs-show-all))
        (t (hs-hide-all))))

After going through this whole “personal preference” thing, it reminded me that the user also has preferences: namely, that the code is “working right.” In particular, they would like to know what it is doing right and wrong. Typically, we rely on the user entering the debugger, which is generally an unpleasant experience easily avoided.

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (cond ((setq gcr-hs-visible (not gcr-hs-visible))
         (hs-show-all)
         (message "Showing all code blocks"))
        (t
         (hs-hide-all)
         (message "Hiding all code blocks"))))

Since I still didn’t handle errors here is how to do it.

1. Capture all errors and let them know explicity: don’t fail on this though, notifying is more important. They will clearly know something failed because it didn’t work. No point in clobbering them over the head with the debugger.
2. Debugging is important: allow debug error handling because if they want it then it is easy to use, just toggle the debug on error feature

If you want to test this add (throw 'foo 'bar) in there.

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (condition-case-unless-debug err
      (cond ((setq gcr-hs-visible (not gcr-hs-visible))
         (hs-show-all)
         (message "Showing all code blocks"))
        (t
         (hs-hide-all)
         (message "Hiding all code blocks")))
    (error (message "An error occurred: %s" (error-message-string err)))))

Speaking of safety but also convenience, it would be nice for the

• The user needs to understand if the mode was loaded because they might not have considered it, and the check is trivially easy.
• Given that it is so easy to check, we might as well check if the package is available.
• Rather then wrapping it in an if block simply to maintain a lispy style of coding just return immediately. This is fine style for how most of know how to program:

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (catch 'return
    (unless (and (boundp 'hs-minor-mode) hs-minor-mode)
      (message (concat "Hideshow either isn't loaded or enabled. "
                       "First load then enable it and try again."))
      (throw 'return nil))
    (condition-case-unless-debug err
        (cond ((setq gcr-hs-visible (not gcr-hs-visible))
               (hs-show-all)
               (message "Showing all code blocks"))
              (t
               (hs-hide-all)
               (message "Hiding all code blocks")))
      (error (message "An error occurred: %s" (error-message-string err))))))

This time I’m really finished:

1. The manual ‘return throw is redundant: can use the built-in cl:return and save that entire block
2. It is weird to have the fall through as t, replace it with a string explaining what is happening. That is self documented code.
3. Explain where the system error occured
4. When the user doesn’t load or enable the library that is actually a user error so report it.

(defvar gcr-hs-visible t "Current state of hideshow for toggling all.

If non-nil all blocks are visible. Else they are hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (unless (and (boundp 'hs-minor-mode) hs-minor-mode)
    (user-error
     (concat "Hideshow either isn't loaded or enabled. "
             "First load then enable it and try again."))
    (cl:return))
  (condition-case-unless-debug err
      (cond ((setq gcr-hs-visible (not gcr-hs-visible))
             (hs-show-all)
             (message "Showing all code blocks"))
            ("Proceed with hide"
             (hs-hide-all)
             (message "Hiding all code blocks")))
    (error
     (message
      "(gcr-hs-visible) Sorry, an error occurred: %s" (error-message-string err)))))

Again, after staring at it so long, I’m haunted by the use of a string as true and also by the use of a boolean to toggle when symbols would explain much better what is happening. This is the final version. Hopefully, it is robust enough for anyone to download it and have it more or less “just work” without ending up in the debugger too much.

• Everything should be in the eror handler block
• Switch state to symbols
• Match on them and fail with a system error unless valid
• Switch from cond to pcase because it is simpler

So that is that, the most simple version.

(defvar gcr-hs-visible 'all-visible "Current state of hideshow for toggling all.

If `\'all-visible' then every block is visible, otherwise it is `\'all-hidden', and they are all hidden.")

(cl-defun gcr-toggle-hideshow-all ()
  "Toggle hideshow all.

  SRC URL: (built in) `hideshow.el.gz'"
  (interactive)
  (condition-case-unless-debug err
      (progn
        (unless (and (boundp 'hs-minor-mode) hs-minor-mode)
          (user-error
           (concat "Hideshow either isn't loaded or enabled. "
                   "First load then enable it and try again."))
          (cl:return))
        (pcase (symbol-value 'gcr-hs-visible)
          ('all-visible
           (setq gcr-hs-visible 'all-hidden)
           (hs-hide-all)
           (message "Hiding all code blocks"))
          ('all-hidden
           (setq gcr-hs-visible 'all-visible)
           (hs-show-all)
           (message "Showing all code blocks"))
          (_
           (error
            (concat "(gcr-hs-visible) Sorry, I couldn't handle toggling "
                    "`%s'. Please notify me.")
            gcr-hs-visible))))
    (error
     (message
      "(gcr-hs-visible) Sorry, an error occurred: %s" (error-message-string err)))))