The most controversial thing about this code is probably my use of def
to change the state of the snake and the apple. It's not yet clear to
me that using atoms is needed here, but I need to think about that
more.
This version has some features that weren't in the original such as:
- automatically turning the snake in the clockwise direction when a
board edge is reached
- changing the color of the snake to black when it overlaps itself
- announcing a win when the length of the snake reaches 10
- automatically restarting the game after an overlap or a win
--
R. Mark Volkmann
Object Computing, Inc.
Calling 'def' to like this is much worse than a lack of comments,
especially in code meant to teach.
--Chouser
Thanks for the feedback! I'll gladly change it to use atoms or agents
or refs. I'm just not sure which one is most appropriate. What do you
think I should use?
Interesting. I can't duplicate that.
> I would use 'cell' instead of 'grid'.
I'll make that change in the next version I post later today.
Thanks for the feedback!
I assume you meant that "get-" is NOT necessary. I considered that,
but I wanted the names of my functions that were not predicates to be
verb-like. For example, that's why I chose the name "make-snake"
instead of "new-snake". I don't know that anyone has documented a
recommended naming convention for Clojure yet, but if there was a
convention I would gladly follow it.
I don't feel I have much authority in the realm of designing
concurrent programs, but here are a couple thoughts:
It seems to me that 'apple' and 'snake' together describe the state of
the game, and therefore need their changes coordinated. This suggests
they should both be refs.
I think it might also be very natural to use a single agent with
Thread/sleep and send-off to itself instead of the Timer. This would
detangle the actionPerformed method from the panel, and allow you to
give the action function a meaningful name instead.
--Chouser
I'm thinking their changes don't need to be coordinated. Here's why.
The only state for an apple is it's color and cell. The color never
changes. The cell only changes when the snake eats it. That is
determined in the actionPerformed method which is also responsible for
triggering movement of the snake, thus changing its state.
actionPerformed is called on the Swing event dispatch thread. That
means there can never multiple threads running actionPerformed
concurrently. I think this is why my code works using def, despite it
being bad form.
> This suggests they should both be refs.
>
> I think it might also be very natural to use a single agent with
> Thread/sleep and send-off to itself instead of the Timer. This would
> detangle the actionPerformed method from the panel, and allow you to
> give the action function a meaningful name instead.
It would also invalidate what I said above about knowing that the
actionPerformed code was always being executed from a single thread.
Do you think it's a good idea to do that anyway?
Perhaps my understanding of the term is a bit off. What makes this
code different from most Clojure code I see is that the functions tend
to be very short and focused. I think this makes reading the code much
easier. I don't feel like I have to think as hard to figure out what
each piece is doing. This makes me more comfortable with including
almost no comments. For example, my previous version contained this:
(paintComponent [graphics]
(proxy-super paintComponent graphics)
(paint graphics @apple (colors :apple))
(doseq [point (:body @snake)]
(paint graphics point (colors :snake))))
My new version contains this:
(paintComponent [graphics]
(proxy-super paintComponent graphics)
(paint-apple graphics)
(paint-snake graphics))
This is a style that is strongly encouraged in Smalltalk, and perhaps
in many other programming communities. It's really just lots of
application of the "extract method" refactoring pattern.
I don't know whether or not you're familiar with the concept of Literate
Programming. If you are, then you can judge for yourself whether that
code qualifies as literate. If not, check out some of these references:
- <http://www-cs-faculty.stanford.edu/~knuth/lp.html>
- <http://en.wikipedia.org/wiki/Literate_programming>
- <http://www.literateprogramming.com/>
- <http://www.literateprogramming.com/knuthweb.pdf>
- <http://vasc.ri.cmu.edu/old_help/Programming/Literate/literate.html>
Many more are out there.
Randall Schulz
But if any other thread were to observe apple and snake immediately
after the apple has been moved (because of a collision), it might be
surprised to find that a moment later the snake has grown even though
the apple was nowhere near it (having already been moved).
Worse yet, suppose some other thread moves the apple, perhaps because
the player is taking too long in driving the snake to it. If this
were to happen after the collision detection but before the apple is
moved, you'd again get unexpected results.
You can know of course that neither of these will happen because there
are no other threads doing any such thing. But (1) that may change in
a future version of the program and (2) even as it is this requires
you to reason about the *entire* program, not just the part you're
dealing with at the moment.
Thus I'd argue that for a correct and robust program, you should use
refs for apple and snake, and think carefully about the appropriate
scope for the dosync(s) that you use to modify them.
> actionPerformed is called on the Swing event dispatch thread.
This means that while the "application logic" part of the program is
running, the UI will not respond to user interaction. Again, this
hardly matters for a toy program, but is something you'd want to avoid
when demonstrating the "right way" to do things.
>> I think it might also be very natural to use a single agent with
>> Thread/sleep and send-off to itself instead of the Timer. This would
>> detangle the actionPerformed method from the panel, and allow you to
>> give the action function a meaningful name instead.
>
> It would also invalidate what I said above about knowing that the
> actionPerformed code was always being executed from a single thread.
> Do you think it's a good idea to do that anyway?
actionPerformed would no longer be run in the Swing thread, and might
indeed be run in different threads at different times, but as long as
it was always run on the same agent, you're guaranteed that no two
actionPerformed would be run at the same time. Actions are serialized
for a given agent.
So yes, I'd still say it's a good idea. Though you would want to use
invokeLater in your new-game function.
--Chouser
Thanks Randall! Clearly what I'm doing doesn't fit the definition of
literate programming. Maybe I can claim that it's "literate style"
based on this part of the definition:
"The main idea is to treat a program as a piece of literature,
addressed to human beings rather than to a computer."
What I'm trying to do is break the code up into a number of helper
functions so the the functions that use them are easier to read. For
example, here's a snippet of my code (including a questionable use of
def that will be changed soon):
(if (snake :alive)
(if (adjacent-or-same-cell? (snake-head) (apple :cell))
(do
(def apple (make-apple))
(move-snake true)
(if (= (snake-length) *length-to-win*)
(new-game "You win!")))
(move-snake false))
(new-game "You killed the snake!"))
I should probably change the arguments to move-snake to be more
meaningful. You get the idea though. What I'm trying to avoid is
deeply nested function definitions with lots of long argument lists
and anonymous functions.
Thanks Chris! Your explanation makes a lot of sense. I'll change the
code as you suggested and put up a new version soon.
Right! Thanks for the suggestion. I've added that.
> Could it make sense to use even fewer def's than currently ?
> I guess it could be made not mandatory to have apple, snake, as global
> vars ?
> And the JFrame too ?
I'm not sure how to get rid of any of those defs. I need access to the
JFrame in the new-game function.
> It could be great to have just defn's (and defstruct's and
> defmacro's), and a single defn entry point for starting the game ?
Are you suggesting that I create a single struct that holds what are
currently global variables in my code? Maybe I should create a global
map named game-config to hold those. Suggestions welcomed!
(defn snake-head [] (first (@snake :body)))
Thanks Brian! I ended up doing something similar that required a
little less code. Here's what I did.
(let [direction (@snake :direction)
head (snake-head)
x (head :x)
y (head :y)
at-left (= x 0)
at-right (= x (- *board-width* 1))
at-top (= y 0)
at-bottom (= y (- *board-height* 1))]
(cond
(and (= direction :up) at-top) (if at-right :left :right)
(and (= direction :right) at-right) (if at-bottom :up :down)
(and (= direction :down) at-bottom) (if at-left :right :left)
(and (= direction :left) at-left) (if at-top :down :up)
true direction)))
I think you're probably right. I wonder though if it's better to make
the callers do the dereference instead of doing it in the snake-head
function. What do others think about this?
> In fact, in most game tutorials, the "game loop" will be in a separate
> thread all together, so it might be more common to leave any def'ing of
> apples and snakes toward the bottom, where you have the gui code.
Yeah, I need to work on making that change.
> As a side note, if I remember correctly, other Java game tutorials usually
> frame up the gui fairly early in the code. They probably do that for
> aesthetic reasons -- so the Java app seems to load faster. Don't quote me
> on that though.
Thanks for the feedback!
What do you think of this way of resolving the issue? Note the use of
the local variable "message". That can get set to either "You win!" or
"You killed the snake!" inside dosync. Then outside dosync I check
that and call new-game only if it's set.
(defn process-move []
(with-local-vars [message nil]
(dosync
(if (@snake :alive)
(if (adjacent-or-same-cell? (snake-head) (@apple :cell))
; Use the next line instead to require collision with the apple.
;(if (= (snake-head) (@apple :cell))
(do
(ref-set apple (make-apple))
(move-snake true)
(if (= (snake-length) *length-to-win*)
(var-set message "You win!")))
(move-snake false))
(var-set message "You killed the snake!"))))
(if (var-get message) (new-game (var-get message)))))
Sounds good. I made that change to mine.
> Passed 'direction' and 'snake-head' to 'new-direction' to avoid accessing
> global state from inside the function
I also made a change to avoid that, but I just passed the dereferenced
snake rather than the direction and head.
> Used destructuring to simplify let statement in new-head (just to see what
> it looked like)
I guess I could do that even though I'm passing the whole snake
struct, but going two levels deep in destructuring is probably
confusing for readers.
> Extracted dy,dx calculation in 'move-snake' into a 'delta' function
That's nice! I'll do that too.
> I notice everything seems to happen in the assignment vector in the 'let' in
> move-snake? Is that good style? I'm not sure.
I'm not either, but it seems pretty readable to me.
> What would be interesting would be to have a second randomly moving snake to
> avoid, that would force out some issues with global state etc.
Yeah, that would be interesting.
> The new code at the end:
> ; Only run the application automatically if run as a script,
> ; not if loaded in a REPL with load-file.
> (println "*command-line-args* =" *command-line-args*)
> (if *command-line-args* (main))
> Doesn't do what it says for me, it printlns and stops from clj, any idea
> what I am doing wrong?
I think it's related to details of your clj script. The relevant like
in mine is this:
java -cp $CP clojure.lang.Script $1 -- $*
I can send you my whole clj script if you'd like.
The latest version of my code that incorporates your changes is at
http://www.ociweb.com/mark/programming/ClojureSnake.html.
Thanks Tom!
It's going off topic, but...
It makes you believe that symbols, like class members in Java, can be
defined in any order and that the existence of the correct symbols
will be properly checked at compile time. Yet this assumption would be
wrong, no?