Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code review from reddit #4

Open
vindarel opened this Issue Mar 11, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@vindarel
Copy link

commented Mar 11, 2019

Hello,

I find your lib interesting. I posted it on reddit and there's a thorough code review which highlights issues: https://www.reddit.com/r/Common_Lisp/comments/azs5hm/listopia_a_list_manipulation_library_port_of/

regards


(defun .intersperse (sep list)
  (labels ((internal-fn (sep list)
             (if (null list)
                 nil
                 (append (list (car list) sep)
                         (internal-fn sep (cdr list))))))
    (.init (internal-fn sep list) nil)))

The function is recursive. Not even tail recursive. Then it calls .init.

(defun .init (list &optional (default nil))
  (if (> (length list) 1)
      (butlast list)
      default))

Which calls LENGTH for every list, which traverses the full list. Cheaper: does the list have atleast two elements?

(defun .concat (list)
  (apply #'concatenate
         (cons 'list list)))

Above is limited to lists with the length of the supported lengths of argument lists. Which is 50 in ABCL.

(defun .scanl (fn init-val list)
  (if (null list)
      (list init-val)
      (cons init-val
            (.scanl fn
                    (funcall fn init-val (car list))
                    (cdr list)))))

.scanl is recursive -> stack overflows...

(defun lazy-list (list fn)
  (vector list fn))

Don't use a vector. Use a structure instead.

(defun .take (count list)
  (when (< count 0) (return-from .take nil))
  (when (lazy-list-p list) (return-from .take (.take-lazy count list)))
  (if (<= count (length list))
      (subseq list 0 count)
      list))

Use COND.

(defun .take-while (pred list)
  (unless (null list)
      (when (funcall pred (first list))
        (cons (first list)
              (.take-while pred (cdr list))))))

Probably better (for example because it won't have stack overflows):

(defun .take-while (pred list)
  (loop for e in list
        while (funcall pred e)
        collect e))

Another one:

(defun .find-indices (pred list)
  (labels ((func (pred list start result)
             (let ((inx (position-if pred list :start start)))
               (if inx
                   (func pred list (1+ inx) (cons inx result))
                   result))))
    (func pred list 0 '())))

(defun .elem-indices (item list)
  (.find-indices (lambda (x) (equalp x item)) list))

better:

(defun .elem-indices (item list)
  (loop for i from 0 and e in list
        when (equal e item) collect i))

(defun .find-indices (pred list)
  (loop for i from 0 and e in list
        when (funcall pred e) collect i)
@Dimercel

This comment has been minimized.

Copy link
Owner

commented Mar 12, 2019

Ok, thanks vindarel. I'm fixing them in the next versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.