3

I'm really new to common Lisp and having some struggles. I'm working on a function that given x, y and an array with the index for vertical value returns NIL if there's any element diagonal from (x y).

(defun diagonal? (x y array)
    (loop for line from 0 to 19 do
        (let (col (aref array line)) (
            (if (= col -1) (return-from diagonal? t))
            (let (diag (= (abs (- x line)) (abs (- y col)))) (
                if (= diag T) (return-from diagonal? NIL))
            )
    )))
    return T
)

When I try this function however, I get the following error:

; caught ERROR:
;   The LET binding spec (AREF ARRAY LINE) is malformed.

;     (SB-INT:NAMED-LAMBDA DIAGONAL?
;         (X Y ARRAY)
;       (BLOCK DIAGONAL?
;         (LOOP FOR LINE FROM 0 TO 19
;               DO (LET (COL #)
;                    (# #)))
;         RETURN
;         T))

2 Answers 2

12

First and extremely important: use automatic indentation.

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let (col (aref array line)) (
                                      (if (= col -1) (return-from diagonal? t))
                                      (let (diag (= (abs (- x line)) (abs (- y col)))) (
                                                                                        if (= diag T) (return-from diagonal? NIL))
                                        )
                                      )))
  return T
  )

Then your code looks strange with long lines: never put parentheses on their own line and never end a line with an open parenthesis.

Improved:

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let (col (aref array line))
          ((if (= col -1)
               (return-from diagonal? t))
           (let (diag (= (abs (- x line))
                         (abs (- y col))))
             (if (= diag T)
                 (return-from diagonal? NIL))))))
  return T)

Second: LET expects a list of bindings. A single binding is a variable or (variable value):

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let ((col (aref array line)))
          ((if (= col -1)
               (return-from diagonal? t))
           (let ((diag (= (abs (- x line))
                          (abs (- y col)))))
             (if (= diag T)
                 (return-from diagonal? NIL))))))
  return T)

Third: LET expects a body of Lisp forms. That is zero or more Lisp forms:

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let ((col (aref array line)))
          (if (= col -1)
               (return-from diagonal? t))
          (let ((diag (= (abs (- x line))
                         (abs (- y col)))))
            (if (= diag T)
                (return-from diagonal? NIL)))))
  return T)

Fourth: = expects numbers as arguments. T is not a number. = already returns T or NIL which we can test.

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let ((col (aref array line)))
          (if (= col -1)
              (return-from diagonal? t))
          (if (= (abs (- x line))
                 (abs (- y col)))
              (return-from diagonal? NIL))))
  return T)

Fifth: return T is not a valid Lisp form. We can just return T directly.

(defun diagonal? (x y array)
  (loop for line from 0 to 19 do
        (let ((col (aref array line)))
          (if (= col -1)
              (return-from diagonal? t))
          (if (= (abs (- x line))
                 (abs (- y col)))
              (return-from diagonal? NIL))))
  T)

Sixth: we don't need the LET for col, we can replace it with another FOR in the LOOP.

(defun diagonal? (x y array)
  (loop for line from 0 to 19
        for col = (aref array line)
        do
        (if (= col -1)
            (return-from diagonal? t))
        (if (= (abs (- x line))
               (abs (- y col)))
            (return-from diagonal? NIL))))
  T)

Seventh: multiple IF can be written as a single COND.

(defun diagonal? (x y array)
  (loop for line from 0 to 19
        for col = (aref array line)
        do (cond ((= col -1)
                  (return-from diagonal? t))
                 ((= (abs (- x line))
                     (abs (- y col)))
                  (return-from diagonal? nil))))
  t)

Eigth: for from 0 to n can be replaced by below (+ n 1) or upto n

(defun diagonal? (x y array)
  (loop for line below 20
        for col = (aref array line)
        do (cond ((= col -1)
                  (return-from diagonal? t))
                 ((= (abs (- x line))
                     (abs (- y col)))
                  (return-from diagonal? nil))))
  t)

Ninth: since (RETURN-FROM ... T) returns from a function which returns T explicitly by default, we can replace it with an UNTIL clause in the loop:

(defun diagonal? (x y array)
  (loop for line below 20
        for col = (aref array line)
        until (= col -1)
        when (= (abs (- x line))
                (abs (- y col)))
        do (return-from diagonal? nil))
  t)

Tenth: since col is just the iterating the values of the array:

(defun diagonal? (x y array)
  (loop for line below 20
        for col across array
        until (= col -1)
        when (= (abs (- x line))
                (abs (- y col)))
        do (return-from diagonal? nil))
  t)

Eleventh: suggestion by @Coredump, use NEVER. The default return value of the LOOP is now T. Return only then nil, when the never clause fails.

(defun diagonal? (x y array)
  (loop for line below 20
        for col across array
        until (= col -1)
        never (= (abs (- x line))
                 (abs (- y col)))))
1
  • 3
    eleventh? use "never" and not "return-from"?
    – coredump
    Oct 24, 2018 at 4:50
6

According to the CLHS a let has the following structure:

(let (var  (var2 expression))
  body ...)

Here the first binding has no value, but it is the same as writing:

(let ((var nil) (var2 expression))
  body ...)

Your bindings look like this:

(let (col                  ; col initialized to nil OK
     (aref array line))    ; variable aref initialized to?
 ...)

Your variable aref should only have one expression. In fact it seems you are lacking a set of parentesis making it look a little like Clojure. Perhaps it should have been:

(let ((col (aref array line)))
  ...)

Also I notice you have a ( on the same line as if you are making a block. That won't work since ((if ....)) is not valid Common Lisp code. You get the error that operator should be a named function or a lambda. let is a block so the start (let ...) makes a block so you can have numerous expressions inside without extra parentheses.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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