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.