CODEWITHSUNDEEP
common-lispclisp
Haruo Wakakusa
Haruo Wakakusa

Reputation: 137

some strategies to refactor my Common Lisp code

I'm Haruo. My pleasure is solving SPOJ in Common Lisp(CLISP). Today I solved Classical/Balk! but in SBCL not CLISP. My CLISP submit failed due to runtime error (NZEC).

I hope my code becomes more sophisticated. Today's problem is just a chance. Please the following my code and tell me your refactoring strategy. I trust you.

https://github.com/haruo-wakakusa/SPOJ-ClispAnswers/blob/0978813be14b536bc3402f8238f9336a54a04346/20040508_adrian_b.lisp

Haruo

Upvotes: 1

Views: 424

Answers (2)

Rainer Joswig
Rainer Joswig

Reputation: 139401

Take for example get-x-depth-for-yz-grid.

(defun get-x-depth-for-yz-grid (planes//yz-plane grid)
  (let ((planes (get-planes-including-yz-grid-in planes//yz-plane grid)))
    (unless (evenp (length planes))
      (error "error in get-x-depth-for-yz-grid"))
    (sort planes (lambda (p1 p2) (< (caar p1) (caar p2))))
    (do* ((rest planes (cddr rest)) (res 0))
         ((null rest) res)
      (incf res (- (caar (second rest)) (caar (first rest)))))))

style -> ERROR can be replaced by ASSERT.

possible bug -> SORT is possibly destructive -> make sure you have a fresh list consed!. If it is already fresh allocated by get-planes-including-yz-grid-in, then we don't need that.

bug -> SORT returns a sorted list. The sorted list is possibly not a side-effect. -> use the returned value

style -> DO replaced with LOOP.

style -> meaning of CAAR unclear. Find better naming or use other data structures.

(defun get-x-depth-for-yz-grid (planes//yz-plane grid)
  (let ((planes (get-planes-including-yz-grid-in planes//yz-plane grid)))
    (assert (evenp (length planes)) (planes)
      "error in get-x-depth-for-yz-grid")
    (setf planes (sort (copy-list planes) #'< :key #'caar))
    (loop for (p1 p2) on planes by #'cddr
          sum (- (caar p2) (caar p1)))))

Upvotes: 4

Dan Robertson
Dan Robertson

Reputation: 4360

  1. Some documentation makes a bigger improvement than refactoring.
  2. Your -> macro will confuse sbcl’s type inference. You should have (-> x) expand into x, and (-> x y...) into (let (($ x)) (-> y...))
  3. You should learn to use loop and use it in more places. dolist with extra mutation is not great
  4. In a lot of places you should use destructuring-bind instead of eg (rest (rest )). You’re also inconsistent as sometimes you’d write (cddr...) for that instead.
  5. Your block* suffers from many problems:
    1. It uses (let (foo) (setf foo...)) which trips up sbcl type inference.
    2. The name block* implies that the various bindings are scoped in a way that they may refer to those previously defined things but actually all initial value may refer to any variable or function name and if that variable has not been initialised then it evaluates to nil.
  6. The style of defining lots of functions inside another function when they can be outside is more typical of scheme (which has syntax for it) than Common Lisp.
  7. get-x-y-and-z-ranges really needs to use loop. I think it’s wrong too: the lists are different lengths.
  8. You need to define some accessor functions instead of using first, etc. Maybe even a struct(!)
  9. (sort foo) might destroy foo. You need to do (setf foo (sort foo)).
  10. There’s basically no reason to use do. Use loop.
  11. You should probably use :key in a few places.
  12. You write defvar but I think you mean defparameter
  13. *t* is a stupid name
  14. Most names are bad and don’t seem to tell me what is going on.
  15. I may be an idiot but I can’t tell at all what your program is doing. It could probably do with a lot of work

Upvotes: 3

Related Questions