Common Lisp formatting/indentation

Question

(defun heapify-down (heap-id 
                     index)
  (if
      (is-leaf heap-id
               index)
      nil
    (let ((left (left-child index)) 
          (right (right-child index))
          (heap-array (heap-actual-heap heap-id)))
      (cond 
       ((null (aref heap-array 
                    left)) 
        (if (= (find-largest heap-id 
                             index 
                             right) 
               index)
            (swap heap-array 
                  index 
                  right))
        nil)
       ((null (aref heap-array 
                    right)) 
        (if (= (find-largest heap-id 
                             index 
                             left) 
               index)
              (swap heap-array 
                    index 
                    left))
        nil)
       ((and (= (find-largest heap-id 
                              index 
                              left) 
                left)
             (= (find-largest heap-id 
                              index 
                              right) 
                right))
        nil)
       (t 
        (let (new-index (find-smallest heap-id 
                                       left 
                                       right))
          (swap heap-array 
                index 
                new-index)
          (heapify-down heap-id 
                        new-index)))))))

So i just wanna know if i'm formatting the right way. i looked some online guides on the topic but i'm not really sure that i'm doing it the best way, especially because my teacher is really strict about the topic.

Answer 1

There is no single parenthesis on a line or various other common problems, and it looks like the forms are aligned correctly, so it is already readable; you tend to add a lot of newlines for no particular reason I think (e.g. after the if?): Common Lisp is often a little more compact vertically. Also note that you have macros for the cases where the if has one branch that is nil, namely when and unless.

I would rewrite it as follows, with small helper functions to make things a little clearer hopefully. You should try to add comments too.

(defpackage :stack-overflow (:use :cl))
(in-package :stack-overflow)

(defun heapify-down (heap-id index)
  (unless (is-leaf heap-id index)
    (let ((left (left-child index)) 
          (right (right-child index))
          (heap-array (heap-actual-heap heap-id)))
      (flet ((get-node (index) (aref heap-array index))
             (swap-with (other) (swap heap-array index other))
             (find-largest (node) (find-largest heap-id index node)))
        (cond
          ;; no left node => swap with right
          ((null (get-node left))
           (when (= index (find-largest right))
             (swap-with right)))

          ;; no right node => swap with left
          ((null (aref heap-array right)) 
           (when (= index (find-largest left))
             (swap-with left)))
          
          ;; both left and right are already sorted
          ((and (= left (find-largest left))
                (= right (find-largest right)))
           nil)
          
          ;; add a new child and heapify down
          (t (let (new-index (find-smallest heap-id left right))
               (swap-with new-index)
               (heapify-down heap-id new-index))))))))

Answer 2

Common Lisp has a built-in pretty printer for code: The Lisp Pretty Printer. See the function PPRINT. It will indent the code and compute a layout for a certain vertical width.

Using it shows you a useful default code layout. Something which a Common Lisp user will recognize as familiar.

Tell it the right margin and it will layout the code for that size.

(defun pretty-print-code (source
                          &key
                          (*print-right-margin* 80)
                          (*print-case* :downcase))
  (pprint source))

Use it with something like

(pretty-print-code '(defun myfunction <some code...>))

Using it for your code, we get using SBCL:

(defun heapify-down (heap-id index)
  (if (is-leaf heap-id index)
      nil
      (let ((left (left-child index))
            (right (right-child index))
            (heap-array (heap-actual-heap heap-id)))
        (cond
         ((null (aref heap-array left))
          (if (= (find-largest heap-id index right) index)
              (swap heap-array index right))
          nil)
         ((null (aref heap-array right))
          (if (= (find-largest heap-id index left) index)
              (swap heap-array index left))
          nil)
         ((and (= (find-largest heap-id index left) left)
               (= (find-largest heap-id index right) right))
          nil)
         (t
          (let (new-index (find-smallest heap-id left right))
            (swap heap-array index new-index)
            (heapify-down heap-id new-index)))))))

LispWorks has slightly different layout rules:

(defun heapify-down (heap-id index)
  (if (is-leaf heap-id index)
      nil
    (let ((left (left-child index))
          (right (right-child index))
          (heap-array (heap-actual-heap heap-id)))
      (cond ((null (aref heap-array left))
             (if (= (find-largest heap-id index right) index)
                 (swap heap-array index right))
             nil)
            ((null (aref heap-array right))
             (if (= (find-largest heap-id index left) index)
                 (swap heap-array index left))
             nil)
            ((and (= (find-largest heap-id index left) left)
                  (= (find-largest heap-id index right) right))
             nil)
            (t
             (let (new-index (find-smallest heap-id left right))
               (swap heap-array index new-index)
               (heapify-down heap-id new-index)))))))

Either one is fine.