[racket] [Newbie] Call for code review

From: Greg Hendershott (greghendershott at gmail.com)
Date: Mon Dec 16 22:57:31 EST 2013

I took a little time to review it. I didn't try to think too deeply
about what it's doing.

Overall I think it looks very, very good. You're "quite new to
Racket"? I'm very impressed!

A few small comments:

1. https://github.com/myguidingstar/game-modeling/blob/master/core.rkt#L18
It would have been nice to have a comment describing what a `world`
is. And maybe why you chose that representation, any pros and cons
that occurred to you.

2. Related: Although I'm not a HTdP zealot, I think it's usually
helpful to write something like a Racket contract as a comment (or
actually use a contract) for non-trivial functions. So perhaps
`create-world` would have a comment like:

;; Given a number of Xs and Os, and a number of rows and columns,
;; creates an initial world with the Xs and Os placed at random
;; coordiantes.
;; number? number? number? number? -> (listof vector?)
(define (create-world n-of-Os n-of-Xs n-of-cols n-of-rows)
  ....

I'd write `exact-positive-integer?` instead of number? if it were
actually a real. But I'm not masochistic enough to type that four
times just for a comment. ;)

3. https://github.com/myguidingstar/game-modeling/blob/master/core.rkt#L24-L36
Be aware that using `when` means that if the condition isn't true,
then `create-world` will return #<void>. Did you maybe want `if` or
`cond` instead, and for the "else" expression return #f or call
`error`?

4. https://github.com/myguidingstar/game-modeling/blob/master/core.rkt#L24-L36
Oh, here's what I mentioned in 1. Maybe just move it higher up in the
file, so a reader can get their bearings?

5. https://github.com/myguidingstar/game-modeling/blob/master/core.rkt#L56-L66
It seems a little awkward for coordinates to be a `(list/c number?
number?)`. It might be nicer simply to pass them as separate x and y
arguments, -or-, to make a `struct` named `coord` or `pos`. (However
it probably doesn't matter too much if you don't work with coords much
more than these couple examples.)


Minor or opinionated comments:

6. Some people (I'm one) prefer using `define` over `let`, usually.
Because indent. However `let` is perfectly readable and "more
portable" if you also do anything with other lisps (Scheme, Elisp,
etc.).

7. https://github.com/myguidingstar/game-modeling/blob/master/.travis.yml
and https://travis-ci.org/myguidingstar/game-modeling
This is running a Ruby env, the Travis default. Doesn't really matter,
I just find it weird to see Ruby output. I like to put `language: c`
as the first line, for a C env.

8. https://github.com/myguidingstar/game-modeling/blob/master/core-test.rkt#L63-L66
You could use `check-true`. Instead of `(check-equal? <something> #t
[<msg>])` you can use `(check-true <something> [<msg>])`.


That's all I have right now. Some of my comments are pretty trivial.
It is nicely written and I'm impressed!



On Mon, Dec 16, 2013 at 6:26 AM, Hoang Minh Thang
<myguidingstar at gmail.com> wrote:
> Hi all,
> I'm quite new to racket. I've made a small program that simulates some
> cells' behaviors in a matrix.
> http://github.com/myguidingstar/game-modeling
> I hope some people will tell me where & how I can improve the code.
> Any single piece is appreciated.
>
> Thanks
>
> ____________________
>   Racket Users list:
>   http://lists.racket-lang.org/users
>

Posted on the users mailing list.