5
\$\begingroup\$

This was my first attempt at writing a program in LISP. Can anyone give any guides as to how it could be improved? The multiple loops in best-pattern seem awkward (I'd normally do that in just one loop but there doesn't seem to be a way of doing that with LOOP, although I'm told there's an iterate extension that can do it), as does the way of calculating alternatives..

(defparameter *rows* (list (list :c :ds :fs :a) (list :cs :e :g :as) (list :d :f :gs :b)))

(defun row-for-note (note)
    (position-if (lambda (row) (find note row)) *rows*)
)

(defun rows-for-notelist (notelist)
    (mapcar #'row-for-note notelist)
)

(defun possible-alternates (rowlist)
    (let ((alternatives '(())))   ;; Lots of irritating silly parentheses
        (when (> (length rowlist) 1)
           (setq alternatives (possible-alternates (cdr rowlist)))
        )
        (case (car rowlist)
            ((0) (mapcar (lambda (x) (append '(0) x)) alternatives))
            ((1) (append (mapcar (lambda (x) (append '(1) x)) alternatives)
                         (mapcar (lambda (x) (append '(-2) x)) alternatives)))
            ((2) (append (mapcar (lambda (x) (append '(2) x)) alternatives)
                         (mapcar (lambda (x) (append '(-1) x)) alternatives)))
        )
    )
)

(defun count-jumps (rowlist)
    (loop for (a b) on rowlist while b counting (> (abs (- a b)) 1))
)

(defun best-pattern (notelist)
    (let ((minval (loop for x in (possible-alternates (rows-for-notelist notelist)) minimize (count-jumps x))))
    (loop for x in (possible-alternates (rows-for-notelist notelist)) when (= (count-jumps x) minval) collect x))
)

(If you're wondering what the program does, it tries to calculate the "most efficient" series of rows to use to play a melody on 5-row Bayan Accordion.)

\$\endgroup\$
3
\$\begingroup\$

Done:

  • improved formatting and layout
  • instead of car and cdr, use first and rest.

Missing:

  • documentation strings
  • example

Only slight improvements.

No need to use list:

(defparameter *rows* '((:c  :ds :fs :a )
                       (:cs :e  :g  :as)
                       (:d  :f  :gs :b )))

Okay:

(defun row-for-note (note)
  (position-if (lambda (row) (find note row))
               *rows*))

(defun rows-for-notelist (notelist)
  (mapcar #'row-for-note notelist))

Compute alternatives directly, without setq. Local functions for repeated use. Use cons. Not so good: recursion limits use, because of limited stack depth.

(defun possible-alternates (rowlist)
  (let ((alternatives (if (rest rowlist)
                          (possible-alternates (rest rowlist))
                        '(()) )))
    (flet ((add (item)
             (mapcar (lambda (x) (cons item x))
                     alternatives)))
      (case (first rowlist)
        (0 (add 0))
        (1 (append (add 1) (add -2)))
        (2 (append (add 2) (add -1)))))))

(defun count-jumps (rowlist)
  (loop for (a b) on rowlist
        while b
        count (> (abs (- a b)) 1)))

Slight improvements:

(defun best-pattern (notelist)
  (let* ((alternates (possible-alternates (rows-for-notelist notelist)))
         (jumps      (mapcar #'count-jumps alternates))
         (min-jump   (loop for j in jumps minimize j)))
    (loop for a in alternates and j in jumps
          when (= min-jump j)
          collect a)))
\$\endgroup\$
  • \$\begingroup\$ Thanks very much! Is there a better way of doing this without recursion? I did spot a few cases where the depth of recursion could be limited (in particular when a note is on row 0 it is always on row 0 so looking ahead beyond it is less important) but that got a bit complex. Is there any way to do best-pattern without looping through the list twice? Oh, and of (mapcar #' some-function somelist) and (loop for x in somelist collecting (some-function x)), is one faster or better in some way? \$\endgroup\$ – Mark Green Dec 29 '13 at 17:45
1
\$\begingroup\$

I'm a Schemer, so my comments are not as detailed as they would be if I were reviewing a Scheme program.

  • First, fix your indentation! There is a Lisp style guide that most Lispers and Schemers agree with. In particular, use an indentation of 2 for functions, and don't use dangling parentheses.
  • Instead of (append '(0) x), just use (cons 0 x). Ditto for all your other append usages too, of course.
  • To reduce your parentheses (your "lots of irritating silly parentheses" did not escape my notice), you can use '(nil) instead of '(()).
\$\endgroup\$
  • \$\begingroup\$ Thanks! I'm really confused by the indentation thing though. The reason I did it the way it's shown above is because I built most of the functions a statement at the time and wanted to easily be able to extend them. I did know about the rule of not having a ) alone on a line but when I did that, then whenever I wanted to add a new statement to a defun or a new case, I ended up having to count parens in order to know where to add the new forms. Splitting them onto single lines made it much easier to extend the code. Is there a better way I'm missing? \$\endgroup\$ – Mark Green Dec 29 '13 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.