Hello fellow lispers,
I'm not a total newbie anymore - I started about 10 months ago - but
I'm probably still far from proficient. Over summer, I took some time
and wrote a slightly-more-than-basic raytracer in LISP. The
common-lisp.net guys were nice enough to host it
(http://common-lisp.net/project/spray/), but I haven't had time to work
on it since the end of summer due to school. So now that the
semester's over, I wanna pick it up again.
But before I start hacking again, I would like some feedback on my code
so far. All the code is here:
http://common-lisp.net/cgi-bin/viewcvs.cgi/spray/?cvsroot=spray#dirlist
(thanks again CL.net!). the "steven" directory is mostly utilities and
stuff, and "steven/raytracer" has all the raytracer code. README has
instructions on running it, if you're interested (you'll need to be in
*nix, with CMUCL installed). It's largely OOP-style, so I use CLOS a
lot.
It's quite a bit of code, but any feedback on any part would be greatly
appreciated! Any design, style, optimization tips, etc would be
awesome. How can I be a better LISP'er and really use the language in
all its glory?
And if you're interested in helping me build this project, lemme know
as well!
Thanks ahead of time
--Steve
········@gmail.com wrote:
> But before I start hacking again, I would like some feedback on my code
> so far.
>
> --Steve
I looked only in matrix.lisp Minor stuff, except (6).
(1) A function defines a block. So in defun mtx-equal you could return-from
mtx-equal. You don't need (block element-checking..). For that matter, I
seldom need to use return-from.
(2) What does (defaliases mtx-equal mtx= mat=) do? If it just does:
(declaim (inline mtx=))
(defun mtx= (x y) (mtx-equal x y)) etc. then I'd go with the latter.
(Another "for that matter") I don't think you are doing anyone a service by
having multiple names for the same function. Others readers of the code
will prefer you stick with one name.
(3) mtx-equal : (if (not (same-dims) nil ...
mtx-equal is a predicate, so why not just put all the conditions in an AND?
(and (same-dims A B)
(domatrix ....)
(if (not (same-dims) nil reads like procedural code
(4) don't use "if" without an "else" Use "when" (still in mtx-equal) ...
You won't need the progn with "when" either.
(5) I wouldn't use a macro for same-dims, num-rows, etc. I'd use an inlined
functions. It's hard to say why to use one over the other (Paul Graham
discussed the tradeoffs in OnLisp (or maybe his "Common Lisp" book).
Essentially, there is no need to manipulate code, so use a function.
(6) Macros like domatrix are going to have a problem if I send a form to be
evaluated for M, that is, if I write (domatrix i j elem (my-function x y
z))... It will evaluate (my-function x y z) multiple times.
You need to use with-gensyms (or some such thing).
(defmacro with-gensyms (syms &body body)
"Paul Graham ON LISP pg 145. Used in macros to avoid variable capture,
etc."
`(let ,(mapcar #'(lambda (s)
`(,s (gensym)))
syms)
,@body))
Here I'll just use a let (like expanding the macro above).
(defmacro domatrix ((i j e M) &body do-form) ; BTW &body for better
indenting
(let ((matrix (gensym)))
`(let ((,matrix ,M)) ;; so eval'ed only once..
(domatrix-by-index (,i ,j ,matrix)....
with-matrix is a better name, btw. This is a with- style macro, it wraps a
body.
--
Best regards,
- Peter