Please review my code
Dom De Felice
) game in Common Lisp.
Being this is my first working program in Common Lisp I would be very
glad to receive comments/opinions/suggestions about the source code
and what should I improve in my CL coding style.
Source code is here:
http://domdefelice.com/projects/scopa/scopa.clisp
It is playable with (play).
(I also wrote a post about it with an example of game session [1]).
Thanks,
dom
[1] http://domdefelice.com/blog/2011/02/scopa-card-game-in-common-lisp/
Zach Beane
> Hello guys, I completed my pet scopa ( http://en.wikipedia.org/wiki/Scopa
> ) game in Common Lisp.
>
> Being this is my first working program in Common Lisp I would be very
> glad to receive comments/opinions/suggestions about the source code
> and what should I improve in my CL coding style.
>
> Source code is here:
> http://domdefelice.com/projects/scopa/scopa.clisp
>
> It is playable with (play).
>
> (I also wrote a post about it with an example of game session [1]).
Case key forms are not evaluated, and may be a list of keys. So, for
example:
(case foo
('bar 1)
('baz 42)
('quux 99))
This actually reads as:
(case foo
((quote bar) 1)
((quote baz) 42)
((quote quux) 99))
If FOO evaluates to the symbol QUOTE, the first clause will always be
selected.
SBCL gives a style warning, at least:
STYLE-WARNING:
Duplicate key QUOTE in CASE form, occurring in the first clause:
((QUOTE BAR) 1), and the second clause:
((QUOTE BAZ) 42).
STYLE-WARNING:
Duplicate key QUOTE in CASE form, occurring in the second clause:
((QUOTE BAZ) 42), and the third clause:
((QUOTE QUUX) 99).
On a tangent, this also means that for a key to match the symbol NIL,
you must use the list form:
(case foo
((nil) 1)
...)
If you just use this:
(case foo
(nil 1)
...)
...it's considered a list of no keys, and will never match.
Zach
Olof-Joachim Frahm
> Being this is my first working program in Common Lisp I would be very
> glad to receive comments/opinions/suggestions about the source code
> and what should I improve in my CL coding style.
Hi Dom, some things I've noticed, that is imho:
- the return value of DELETE(-IF) has to be used, it doesn't necessarily
modify the given sequence, or it might remove the first element of the
sequence,
- GAME-STEP calls itself and PLAY recursively, which isn't guaranteed to
be optimized away by a tail-call optimization, so do an explicit loop
in PLAY or on another outer layer,
and more stylistic things:
- add an option to quit the game (user input 0?) (and perhaps check
for wrong input too ...),
- multiple SETF clauses can be written in one statement, e.g.
> (setf (player-scopas human) 0
> (player-scopas pc) 0)
- "with i = 1 do ... (incf i)" is the same as "for i from 1 do"
- PRINC and TERPRI together probably aren't as clear as a single FORMAT
call; in any case, a single FORMAT call is easier to edit later on,
- you could use PRINT-OBJECT instead of implementing separate DESCRIBE-
functions, e.g. specialize it on the structs GAME, PLAYER, CARD, then
simply print the object on the output stream to describe them,
- since you already use formatstrings quite often, you could also
rewrite DESCRIBE-OPTION using a conditional as:
> (defun describe-option (option)
> (let ((played-card (car option))
> (collected-cards (cdr option)))
> (format t "Play ~A~@[ taking ~{~A~^, ~}~]~%"
> (card-description played-card)
> (mapcar #'card-description collected-cards))))
- move the comments into docstrings, e.g. for POSSIBLE-OPTIONS or
-PLAYING the comment already describes the function very detailed,
- is there another reason to use a list in HAND-SCORES-ASSIGMENT other
than the COUNT calls in UPDATE-SCORES? I'd use a structure here too.
- FROM-DECK and INTO-DECK could be called with unquoted symbols
(like (from-deck hand ...)) if you move the quote to the macro
definition, e.g. write them as
> (defmacro from-deck (deck &body body)
> `(let ((*from-deck* (deck ',deck)))
> ,@body))
Hopefully I've made no errors ... anyway, regards and hth,
Olof
Dom De Felice
> Case key forms are not evaluated, and may be a list of keys. So, for
> example:
>
> (case foo
> ('bar 1)
> ('baz 42)
> ('quux 99))
>
> This actually reads as:
>
> (case foo
> ((quote bar) 1)
> ((quote baz) 42)
> ((quote quux) 99))
>
> If FOO evaluates to the symbol QUOTE, the first clause will always be
> selected.
Uhm didn't know that!
Corrected :-)
> Zach
dom
Dom De Felice
> Hi Dom, some things I've noticed, that is imho:
> and more stylistic things:
>[...]
> Hopefully I've made no errors ... anyway, regards and hth,
Today I will refactor my code following your tips :-)
> Olof
dom