And now a slight diversion…

So rather than continuing on with our next section, I’m going to take a moment and look at some refactoring I did to several bits of code.

I would say that some of the issues with the original code stem from me being relatively new to lisp—I don’t always recognize the potential idiomatic versions of things, or I don’t have a visceral notion of exactly what a particular function is doing.

The other source is probably just that this code changed over time, and I didn’t always take the time, or see the opportunity to refactor at each step. I will say, though, that I am inordinately happy to have a fairly comprehensive test suite that immediately identified problematic transformations in the code. Without that, I would have been much less confident of my results.

So, without further ado, here’s the first function we’ll refactor, in its original form:

(defun org-blog-post-to-wp (post)
  "Transform a post into a structure for submitting to WordPress.

This is largely about mapping tag names, though the handling of
`category' and `tags' is little more complex as the WordPress API
now groups them as `taxonomies', and requires a hierarchical
structure to differentiate them.

For convenience in testing and inspection, the resulting alist is
sorted."
  (sort
   (reduce
    '(lambda (wp new)
       (let ((k (car new))
             (v (cdr new)))
         (when v
           (cond ((eq :category k)
                  (setq wp (org-blog-post-to-wp-add-taxonomy wp "category" v)))
                 ((eq :date k)
                  ;; Convert to GMT by adding seconds offset
                  (push (cons "post_date_gmt" (list :datetime
                                                    (time-add (car v)
                                                              (seconds-to-time (- (car (current-time-zone)))))))
                        wp))
                 ((eq :tags k)
                  (setq wp (org-blog-post-to-wp-add-taxonomy wp "post_tag" v)))
                 ((eq :title k)
                  (push (cons "post_title" (or v "No Title")) wp))
                 ((assq k org-blog-wp-alist)
                  (push (cons (cdr (assq k org-blog-wp-alist)) v) wp))
                 )))
       wp)
    post :initial-value nil)
   '(lambda (a b)
      (string< (car a) (car b)))))

So, my big “Aha!” moment was when I happened to go back and re-read the documentation for push, which has this observation embedded in it:

[…] and this macro is equivalent to (setq listname (cons element listname)).

Now this might not immediately suggest any particular refactoring, but I also noticed that the two cases that weren’t using push, were using (setq wp), and in both of those cases, (:category and :tags), we’re setting it to the output of a function, and we’re using the modified value of wp as the return value of our lambda.

Knowing that a lisp form generally returns the value of the last statement that it executes, and that both setq and push return their modified values, my first simplification was to simply remove the lonely little wp at the end of the lambda, and let the result of the cond be the return value from the lambda. Which promptly broke things—I was getting partial structures back.

What I hadn’t taken into account was that when there was no value in v, the when statement would return no value—so any attribute that didn’t have a value would reset reduce‘s accumulator to nil.

That was easily enough solved by converting the when to an if that returned the unmodified wp if there was no value to be added. This got me:

(lambda (wp new)
  (let ((k (car new))
        (v (cdr new)))
    (if v
        (cond ((eq :category k)
               (setq wp (org-blog-post-to-wp-add-taxonomy wp "category" v)))
              ((eq :date k)
               ;; Convert to GMT by adding seconds offset
               (push (cons "post_date_gmt" (list :datetime
                                                 (time-add (car v)
                                                           (seconds-to-time (- (car (current-time-zone)))))))
                     wp))
              ((eq :tags k)
               (setq wp (org-blog-post-to-wp-add-taxonomy wp "post_tag" v)))
              ((eq :title k)
               (push (cons "post_title" (or v "No Title")) wp))
              ((assq k org-blog-wp-alist)
               (push (cons (cdr (assq k org-blog-wp-alist)) v) wp)))
      wp)))

That seemed like just moving the food around on the plate—hardly worth it. However, I then realized a further change—that everything could go in the cond, and the if could be eliminated entirely. This produces:

(lambda (wp new)
  (let ((k (car new))
        (v (cdr new)))
    (cond ((eq v nil)
           wp)
          ((eq :category k)
           (setq wp (org-blog-post-to-wp-add-taxonomy wp "category" v)))
          ((eq :date k)
           ;; Convert to GMT by adding seconds offset
           (push (cons "post_date_gmt" (list :datetime
                                             (time-add (car v)
                                                       (seconds-to-time (- (car (current-time-zone)))))))
                 wp))
          ((eq :tags k)
           (setq wp (org-blog-post-to-wp-add-taxonomy wp "post_tag" v)))
          ((eq :title k)
           (push (cons "post_title" (or v "No Title")) wp))
          ((assq k org-blog-wp-alist)
           (push (cons (cdr (assq k org-blog-wp-alist)) v) wp)))))

While it’s not really shorter, I think it’s clearer—all inspection of the item we’re working with is done in the cond, rather in a combination of an initial conditional (our if / when) and then the cond.

There’s one last little bit of cleanup that’s worth doing. It circles back around to the initial observation about push. That is, if we’re just using the return value of push, which is really the return value of the implicit setq, why do we need push / setq at all? They’re just noise and extra operations. So we can transform any push to a simple cons, and eliminate our setq calls entirely.

(lambda (wp new)
  (let ((k (car new))
        (v (cdr new)))
    (cond ((eq v nil)
           wp)
          ((eq :category k)
           (org-blog-post-to-wp-add-taxonomy wp "category" v))
          ((eq :date k)
           ;; Convert to GMT by adding seconds offset
           (cons (cons "post_date_gmt" (list :datetime
                                             (time-add (car v)
                                                       (seconds-to-time (- (car (current-time-zone)))))))
                 wp))
          ((eq :tags k)
           (org-blog-post-to-wp-add-taxonomy wp "post_tag" v))
          ((eq :title k)
           (cons (cons "post_title" (or v "No Title")) wp))
          ((assq k org-blog-wp-alist)
           (cons (cons (cdr (assq k org-blog-wp-alist)) v) wp)))))

Again, it’s not dramatically shorter, but it is much more focused—it is doing what it intends with as little extra fuss as possible. And, again, it’s more about transformations than imperative operations—where we used to be setting variables and pushing things onto them, now we’re running functions and returning their output, or creating a new items by prepending a cons cell on an existing structure.

Even more interestingly, once I refactored this function, I started looking at a number of other places where I was able to make the same sort of changes—moving from a more imperative style to one where I was more willing to trust the output of my functions to carry me through.