Account Options

  1. Sign in
The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Google Groups Home
« Groups Home
Please review my code
There are currently too many topics in this group that display first. To make this topic appear first, remove this option from another topic.
There was an error processing your request. Please try again.
flag
  5 messages - Collapse all  -  Translate all to Translated (View all originals)
The group you are posting to is a Usenet group. Messages posted to this group will make your email address visible to anyone on the Internet.
Your reply message has not been sent.
Your post was successful
 
From:
To:
Cc:
Followup To:
Add Cc | Add Followup-to | Edit Subject
Subject:
Validation:
For verification purposes please type the characters you see in the picture below or the numbers you hear by clicking the accessibility icon. Listen and type the numbers you hear
 
Dom De Felice  
View profile  
 More options Feb 7 2011, 2:32 pm
Newsgroups: comp.lang.lisp
From: Dom De Felice <dom.fel...@gmail.com>
Date: Mon, 7 Feb 2011 11:32:23 -0800 (PST)
Local: Mon, Feb 7 2011 2:32 pm
Subject: Please review my code
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]).

Thanks,
dom

[1] http://domdefelice.com/blog/2011/02/scopa-card-game-in-common-lisp/


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Zach Beane  
View profile  
 More options Feb 7 2011, 2:40 pm
Newsgroups: comp.lang.lisp
From: Zach Beane <x...@xach.com>
Date: Mon, 07 Feb 2011 14:40:19 -0500
Local: Mon, Feb 7 2011 2:40 pm
Subject: Re: Please review my code
Dom De Felice <dom.fel...@gmail.com> writes:

> 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


 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Olof-Joachim Frahm  
View profile  
 More options Feb 7 2011, 6:42 pm
Newsgroups: comp.lang.lisp
From: Olof-Joachim Frahm <Olof.Fr...@web.de>
Date: Tue, 08 Feb 2011 00:42:27 +0100
Local: Mon, Feb 7 2011 6:42 pm
Subject: Re: Please review my code
Dom De Felice <dom.fel...@gmail.com> writes:

> 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

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Dom De Felice  
View profile  
 More options Feb 9 2011, 12:53 am
Newsgroups: comp.lang.lisp
From: Dom De Felice <dom.fel...@gmail.com>
Date: Tue, 8 Feb 2011 21:53:39 -0800 (PST)
Local: Wed, Feb 9 2011 12:53 am
Subject: Re: Please review my code
On Feb 7, 8:40 pm, Zach Beane <x...@xach.com> wrote:

Uhm didn't know that!
Corrected :-)

> Zach

dom

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
Dom De Felice  
View profile  
 More options Feb 9 2011, 12:55 am
Newsgroups: comp.lang.lisp
From: Dom De Felice <dom.fel...@gmail.com>
Date: Tue, 8 Feb 2011 21:55:23 -0800 (PST)
Local: Wed, Feb 9 2011 12:55 am
Subject: Re: Please review my code
On Feb 8, 12:42 am, Olof-Joachim Frahm <Olof.Fr...@web.de> wrote:
> Hi Dom, some things I've noticed, that is imho:
>[...]
> and more stylistic things:
>[...]
> Hopefully I've made no errors ... anyway, regards and hth,

Wow, thanks for all the good suggestions!
Today I will refactor my code following your tips :-)

> Olof

dom

 
You must Sign in before you can post messages.
To post a message you must first join this group.
Please update your nickname on the subscription settings page before posting.
You do not have the permission required to post.
End of messages
« Back to Discussions « Newer topic     Older topic »