CODEWITHSUNDEEP
lispcommon-lisp
Klladdy
Klladdy

Reputation: 11

Confused by Lisp list comparison

Below I'm having a with my Lisp program with a comparison between a list and a list. One is generated from user input and one is part of table that is pre-generated. I test the function with set input separately; there, (isValidMove '(0 0)) returns T, yet when build a list for comparisons using query-io I get false. Comparison, in any language, has always given me trouble because of how many different things are identical to me but apparently vastly different to the computer; I assume that's the same problem I'm having here. (By the way, I'm including only a part of a larger program). 

;Local Variables (program wide)
;Board values stores x/o and defaults to " "
(setf boardValues (make-array '(3 3)
    :initial-element " ")
);end boardValues

;List of all valid moves remaining 
(setf validMoves (list 
    (list 0 0) (list 0 1) (list 0 2) 
    (list 1 0) (list 1 1) (list 1 2) 
    (list 2 0) (list 2 1) (list 2 2)))

;Functions

;Function call the will prompt the user for input, if the move is
;not vaild, repromts for a move
(defun getUserMove ()

    (let ((move (read-line *query-io*)))
    (if (isValidMove move)
        (progn
            (setf (aref validMoves (car move) (cdr move)) 'x)
            (remove move validMoves))
            (getUserMove)))
);end getUserMove

;Function call that process the move, returns T if move is valid
;and F if move is invalid

(defun isValidMove (move)
    (dolist (m validMoves) 
    (if (equalp m move)
        (return T)))
) ;end isValidMove

Upvotes: 0

Views: 130

Answers (2)

mobiuseng
mobiuseng

Reputation: 2396

Let's go through the code fixing problems:

(setf boardValues (make-array '(3 3)
    :initial-element " ")
)

Do not use SETF to declare global variables. Use DEFVAR or DEFPARAMETER. Global variables are special: to indicate this, use "ear-muffs". Also, you don't need comments in Common Lisp telling what a variable or a function is for: use doc-strings when defining them:

(defvar *board-values* (make-array '(3 3) :initial-element " ")
  "Board values stores x/o and defaults to \" \"")

Also, use Common Lisp naming conventions (no camel case, use - to separate words; IMHO Lisp's convention is way more readable).

The same issue with VALIDMOVES. Replace your definition with:

(defvar *valid-moves*
  (list '(0 0) '(0 1) '(0 2) 
        '(1 0) '(1 1) '(1 2) 
        '(2 0) '(2 1) '(2 2))
  "List of all valid moves remaining ")

Now, the change in the function is more involved:

(defun get-user-move ()
  (let ((*read-eval* nil))
    (let ((move (read-from-string (read-line *query-io*))))
      (when (valid-move-p move)
        (setf (aref *board-values* (car move) (cadr move)) 'x)
        (setf *valid-moves* (remove move *valid-moves* :test #'equalp))
        (get-user-move)))))

Let's look at it line-by-line:

  1. Use CL name convention
  2. We will be reading from the string. CL has reader macros, powerful but dangerous things. By setting *READ-EVAL* to NIL we suppress the evaluation that can happen at read-time.
  3. Read user input into variable MOVE: READ-LINE will take user input and return a string of the input, READ-FROM-STRING will convert the string into Lisp-data. So if user inputs "(1 2)", MOVE will contain the list (1 2).
  4. We perform multiple actions on the valid move and do nothing if it isn't valid. Better use WHEN instead of IF in this case: won't need PORGN and show more precisely what we are doing compared to IF. It is more idiomatic in CL to name the predicate with -P ending instead of having IS in front, thus ISVALIDMOVE is renamed to VALID-MOVE-P.
  5. Here I assume the original program had an error: you were calling AREF on VALIDMOVES which is not an array. I assume you wanted to change the state of the board. Notice that coordinates are taken using CAR and CADR of the MOVE: MOVE is the list not just a pair, in effect it has a form (CONS X (CONS Y NIL)). So, to get Y need to take (CAR (CDR MOVE)) or (CADR MOVE) or (SECOND MOVE); (CDR MOVE) will return (Y) - not a valid index for an array.
  6. REMOVE does not affect original sequence! Even if you use destructive DELETE you shouldn't rely on it changing the original sequence, but use the returned value. Thus, need to re-assign *VALID-MOVES* value after removing the move. Also, need to supply a different equality test for REMOVE since (EQL (LIST 1 2) (LIST 1 2)) is NIL.
  7. Recursively call itself. Perhaps, more idiomatic CL code would use LOOP, but there is nothing wrong with recursion here.

NB: I have changed the semantic of a function a bit: it returns on an invalid move. Otherwise, the user has to interrupt the evaluation to stop. Even better would be to have a special input for exit (symbol QUIT or Q?) and re-prompt for input in case of invalid move.

Finally, function VALID-MOVE-P is quite trivial in CL:

(defun valid-move-p (move)
  (member move *valid-moves* :test #'equalp))

Instead of manually walking through the list, can use primitive (in a sense "provided by CL standard") function MEMBER.

Now it works:

CL-USER> (get-user-move)
(0 1)
(0 2)
(10 20)
NIL
CL-USER> *board-values*
#2A((" " X X) (" " " " " ") (" " " " " "))
CL-USER> *valid-moves*
((0 0) (1 0) (1 1) (1 2) (2 0) (2 1) (2 2))

PS. The code is still far from the ideal but it works.

Upvotes: 1

verdammelt
verdammelt

Reputation: 942

In GETUSERMOVE you bind MOVE to be the result of evaluating READ-LINE. READ-LINE returns a string not a list. (ref: http://www.lispworks.com/documentation/HyperSpec/Body/f_rd_lin.htm)

Thus ISVALIDMOVE can never evaluate to T given that a string will never be EQUALP to a list.

To transform your string into a list you will need to call EVAL. But be cautious about using EVAL on user inputted data!

Upvotes: 1

Related Questions