Hi,
I am trying to take my lisp education a little more seriously and was
wondering if any one could offer suggestion or point out major problems
with the lisp code found at
http://www.jeremyenglish.org/lisp-stuff.html
Comments would be greatly appreciated.
Jeremy
··············@gmail.com writes:
> http://www.jeremyenglish.org/lisp-stuff.html
>
> Comments would be greatly appreciated.
I had a look at music-theory.lisp, and overall it looked quite good to
me. I'll offer some minor comments:
> (defparameter *notes* '(A (A# Bb) B C (C# Cb) D (D# Eb) E F (F# Gb) G (G# Ab)))
>
> (defun find-note (note)
> (let ((N nil))
> (labels ((do-search (notes i)
> (cond
> ((null notes) nil)
> ((and (atom (car notes)) (eq N (car notes))) i)
> ((and (consp (car notes))
> (or (eq N (first (car notes)))
> (eq N (second (car notes))))) i)
> (t (do-search (cdr notes) (+ i 1))))))
> (if (consp note) (setf N (car note)) (setf N note))
> (do-search *notes* 0))))
I suspect this does the job more readably:
(defparameter *notes* '((A) (A# Bb) (B) (C) (C# Cb) (D) (D# Eb) (E) (F) (F# Gb) (G) (G# Ab)))
(defun note-index (note)
(position-if (lambda (set) (member note set)) *notes*))
> (defmacro make-scale ...)
Stylistically, I'd suggest giving declarative operators such as
this names that begin with "define-". I'd also make an effort to avoid
capital letters as variable names.
--
Frode Vatvedt Fjeld
In article <························@u72g2000cwu.googlegroups.com>,
··············@gmail.com wrote:
> Hi,
>
> I am trying to take my lisp education a little more seriously and was
> wondering if any one could offer suggestion or point out major problems
> with the lisp code found at
> http://www.jeremyenglish.org/lisp-stuff.html
>
> Comments would be greatly appreciated.
>
> Jeremy
A bit the palindromic-panagram.lisp
Using global variables is a bit out of fashion.
APPEND-FILE
- use WITH-OPEN-FILE
- you don't need (WHEN f ...
Don't use global-variables all over in the functions.
Just use them in the interface if possible.
(defun to-log (item &key (log-file *log-file*))
"Appends item onto log-file"
(when log-file
(append-file item log-file)))
Replace the FOREACH macro with
(loop for item across vector
do ...)
Don't use assignments. Binding is enough. Expressions
return values. Put unlikely alternatives to the end.
(defun fn-starts-with (pattern str)
"Check to see if the string str starts with pattern."
(let ((sub (if (>= (length str) (length pattern))
(subseq str 0 (length pattern))
str)))
(cond
((string< pattern sub) 'less)
((string> pattern sub) 'greater)
((string= pattern sub) 'equal))))
Again, get rid of assignments. Move lets
to their minimum scope.
(defun partial-match (s matcher trimmer)
"Keep triming the pattern until we get a partial match"
(when s
(let ((l (funcall matcher s)))
(if (plusp (length l))
(cons (length s) l)
(partial-match (funcall trimmer s) matcher trimmer)))))
Again, a more Functional version:
(defun dup-word? (w side)
"Check to see if we have used this word"
(binary-search-vector
w
(sort (copy-seq (if (equal side 'right) *right-side* *left-side*))
#'string<)
#'match-all))
This is inefficient:
(defun vector-to-string (vec)
"Concatenate all of the elements of vector onto a string."
(let ((s "")
(w ""))
(foreach (w vec) (setf s (concatenate 'string s w)))
s))
....
--
http://lispm.dyndns.org/