Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm studying Common Lisp on my own. Coming from C++, Common Lisp feels kind of strange sometimes. I'd like to get some feedback on what I understood so far.

For learning purposes, I wrote this simple vector class package. The only requirements apart from correctness are non-destructiveness and performance. I want it to be as fast as possible. Is this the way to do it? Are there any serious flaws?

(in-package :cl-user)
(defpackage math
  (:use :cl)
  (:export :vector3
       :create-vector3
       :vector3-x
       :vector3-y
       :vector3-z
       :neg
       :mul
       :div
       :add
       :sub
       :dot
       :len
       :norm))
(in-package :math)

(defclass vector3 ()
  ((x :accessor vector3-x
      :initarg :x
      :initform 0
      :type double-float)
   (y :accessor vector3-y
      :initarg :y
      :initform 0
      :type double-float)
   (z :accessor vector3-z
      :initarg :z
      :initform 0
      :type double-float)))

(defun create-vector3 (x y z)
  (make-instance 'vector3 :x x :y y :z z))

(defmethod neg ((v vector3))
  (create-vector3 (- (vector3-x v))
          (- (vector3-y v))
          (- (vector3-z v))))

(defmethod mul ((s double-float) (v vector3))
  (create-vector3 (* s (vector3-x v))
          (* s (vector3-y v))
          (* s (vector3-z v))))

(defmethod div ((v vector3) (s double-float))
  (create-vector3 (/ (vector3-x v) s)
          (/ (vector3-y v) s)
          (/ (vector3-z v) s)))

(defmethod add ((a vector3) (b vector3))
  (create-vector3 (+ (vector3-x a) (vector3-x b))
          (+ (vector3-y a) (vector3-y b))
          (+ (vector3-z a) (vector3-z b))))

(defmethod sub ((a vector3) (b vector3))
  (create-vector3 (- (vector3-x a) (vector3-x b))
          (- (vector3-y a) (vector3-y b))
          (- (vector3-z a) (vector3-z b))))

(defmethod dot ((a vector3) (b vector3))
  (+ (* (vector3-x a) (vector3-x b))
     (* (vector3-y a) (vector3-y b))
     (* (vector3-z a) (vector3-z b))))

(defmethod len ((v vector3))
  (sqrt (+ (* (vector3-x v) (vector3-x v))
       (* (vector3-y v) (vector3-y v))
       (* (vector3-z v) (vector3-z v)))))

(defmethod norm ((v vector3))
  (let ((l (len v)))
    (if (> l 0)
    (let ((inv (/ 1.0 l)))
      (create-vector3 (* (vector3-x v) inv)
              (* (vector3-y v) inv)
              (* (vector3-z v) inv)))
    (create-vector3 (vector3-x v)
            (vector3-y v)
            (vector3-z v)))))
share|improve this question
1  
Start with using the time macro, check out how to disassemble functions, and try to find a good profiler for your lisp implementation –  Clayton Stanley Jul 8 '12 at 2:50

1 Answer 1

Here are a few minor suggestions:

  1. Use uninterned symbols instead of keywords in defpackage (e.g., #:neg instead of :neg) to avoid polluting the keyword package.
  2. Fix your indentation.
  3. Add a defgeneric with docs for each defmethod (you can fold defmethod inside defgeneric)
  4. Decide if your objects are read-only; if they are, replace :accessor with :reader, if they are not, add mutator functions (e.g., scale in-place)
  5. Rename norm to normalize (norm in math refers to the length of the vector)
  6. Use (plusp x) instead of (> x 0)
  7. Use (/ x) instead of (/ 1 x)
  8. In the else clause in norm can just return the argument or (create-vector3 0 0 0) for clarity
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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