First off, obviously I care about the future of Clojure or I wouldn't
have spent the last six weeks or so trying to learn it.
I imagine that almost everyone on this mailing list would like to have
the opportunity to spend more time on the job coding in Clojure than
they get to currently. One way to make that more likely is to make it
easier for others to learn Clojure. You may not be allowed to use it
for production code if you're the only one in your company that can
understand it.
Many examples of Clojure code work counter to this goal. This snake
code is just one of them. I don't mean to pick on the authors. This
code is very similar to other Clojure code I encounter daily.
Here are some things I think we could all do in our Clojure code to
make it easier for others to understand.
1) There are no prizes for using one letter variable names. Pick more
meaningful names.
For example, (defn collision? [{[b] :body} a] ...).
What is a? It's a vector containing the x/y coordinates of the apple
the snake is trying to eat. Why not name it "apple" or
"apple-location"?
What is b? It's a vector that represents the x/y coordinates of the
head of the snake.
Why not name it "head" or "snake-head"?
2) There are no prizes for writing code with zero comments. One of the
strengths of Clojure is that you can accomplish a large amount in a
small amount of code. That doesn't mean that readers of your code will
know what is going on though.
For example, what does this do?
(every? #(<= (- (a %) 10) (b %) (+ 10 (a %))) [0 1])
Well, 0 and 1 are indexes of the x and y coordinates represented by
two element vectors that represent the location of the apple and the
head of the snake. This tests whether every coordinate (the x and y)
of the apple are "close" to the corresponding coordinate of the snake
head. This certainly needs to be explained in a comment.
3) There are no prizes for cramming loads of functionality into a
single line. Spread it out to make it easier to read.
For example, which of these is easier to understand?
; original
(assoc snake :body (cons (vec (map #(+ (dir %) ((first body) %)) [0 1]))
(if grow body (butlast body)))))
What is being cons'ed to what here?
; modified
(assoc snake :body
(cons
(vec (map #(+ (dir %) ((first body) %)) [0 1]))
(if grow body (butlast body))
)
)
I know my placement of the closing parens on separate lines is
non-standard in the Lisp world, but I find it helps me see better
where constructs end. Without doing that it feels like Python where
indentation is significant. I could concede the paren placement
though.
Below is the original code. Compare it to my version at
http://pastie.org/348031 where variables are renamed, comments are
added, and indentation is changed in a way that I feel makes it more
readable.
(import '(java.awt Color) '(javax.swing JPanel JFrame Timer)
'(java.awt.event KeyEvent ActionListener KeyListener))
(defn gen-apple [_] [(rand-int 750) (rand-int 550)])
(defn move [{:keys [body dir] :as snake} & grow]
(assoc snake :body (cons (vec (map #(+ (dir %) ((first body) %)) [0 1]))
(if grow body (butlast body)))))
(defn turn [snake newdir] (if newdir (assoc snake :dir newdir) snake))
(defn collision? [{[b] :body} a]
(every? #(<= (- (a %) 10) (b %) (+ 10 (a %))) [0 1]))
(defn paint [g p c] (.setColor g c) (.fillRect g (p 0) (p 1) 10 10))
(def dirs {KeyEvent/VK_LEFT [-10 0] KeyEvent/VK_RIGHT [10 0]
KeyEvent/VK_UP [0 -10] KeyEvent/VK_DOWN [0 10]})
(def apple (atom (gen-apple nil)))
(def snake (atom {:body (list [10 10]) :dir [10 0]}))
(def colors {:apple (Color. 210 50 90) :snake (Color. 15 160 70)})
(def panel (proxy [JPanel ActionListener KeyListener] []
(paintComponent [g] (proxy-super paintComponent g)
(paint g @apple (colors :apple))
(doseq [p (:body @snake)]
(paint g p (colors :snake))))
(actionPerformed [e] (if (collision? @snake @apple)
(do (swap! apple gen-apple)
(swap! snake move :grow))
(swap! snake move))
(.repaint this))
(keyPressed [e] (swap! snake turn (dirs (.getKeyCode e))))
(keyReleased [e])
(keyTyped [e])))
(doto panel (.setFocusable true)(.addKeyListener panel))
(doto (JFrame. "Snake")(.add panel)(.setSize 800 600)(.setVisible true))
(.start (Timer. 75 panel))
Another example of a fairly short piece of code that is begging for
comments is the example at http://clojure.org/concurrent_programming.
Imagine someone who has only been learning Clojure for a couple of
days and is curious about how Clojure handles concurrency trying to
follow what is happening in that code.
So my main point is that if we all make an effort to make our code
easier to read, it will be easier to convince other developers to
consider learning Clojure which will be good for all of us.
--
R. Mark Volkmann
Object Computing, Inc.
I agree ... if they are really redundant. For example, I don't feel
that explaining the use of [0, 1] to represent indexes for x and y
values is redundant.
> Trailing parens are known bad - don't use them.
I'm not disagreeing, but I'd like to hear the explanation for why they
are bad. The ones that end defn's seem the same as Java's trailing
braces to me.
> Though when first learning you might wish every piece of code had a
> built-in language tutorial, I'd be dismayed if, e.g., every use of
> proxy contained this redundant description of how it works:
>
> ; Create a proxy object that extends JPanel and
> ; implements both ActionListener and KeyListener.
> (proxy [JPanel ActionListener KeyListener]
> [] ; arguments to the superclass constructor (none here)
>
> ; What follows are implementations of the interface methods.
Right. I agree with you there and I did put those in my code because
that was my first exposure to Clojure proxies.
> etc. Most of your comments don't say anything that the code doesn't.
> Those that do may be worthwhile, those that don't should not be in non-
> tutorial code.
>
> The original code was not intended as a tutorial, nor is most code,
> nor should it be.
>
> 'X in Y lines of code' examples are inherently trying to be concise,
> and probably not the best things to learn from initially.
Even a Clojure expert would likely pause for a minute to figure out
what is going on in this code in the absence of any comments.
I still think there's a benefit in experienced Clojure developers
taking the extra effort to make their code more accessible to Clojure
newbies, especially if we want the community to grow.
I apologize for the nature of my email which somewhat singled you out.
I would like to produce a version of the snake code that could serve
as an example of the kind of code that the Clojure community thinks is
"good". Unless it's part of an exercise to produce the shortest code
possible, I think we should always write Clojure code with a goal of
making it as easy as possible for others to read, while not attempting
to serve as a Clojure tutorial. Again, my goal here is to get more
developers to give Clojure a shot.
My challenge to everyone on the list is to start with any version of
the snake code you've seen and make it as readable as *you* think it
should be by doing things like renaming variables and functions,
adding comments and changing indentation. I'd really like to see what
*you* think is the best way to write this code. The lessons learned
from this exercise could then be applied to other code we write in the
future.
> I would like to produce a version of the snake code that could serve
> as an example of the kind of code that the Clojure community thinks is
> "good". Unless it's part of an exercise to produce the shortest code
> possible, I think we should always write Clojure code with a goal of
> making it as easy as possible for others to read, while not attempting
> to serve as a Clojure tutorial. Again, my goal here is to get more
> developers to give Clojure a shot.
>
> My challenge to everyone on the list is to start with any version of
> the snake code you've seen and make it as readable as *you* think it
> should be by doing things like renaming variables and functions,
> adding comments and changing indentation. I'd really like to see what
> *you* think is the best way to write this code. The lessons learned
> from this exercise could then be applied to other code we write in the
> future.
Okay, I took the challenge and produced a modified version of my
earlier code where I removed what I considered to be redundant
comments and did a little more renaming. You can see it at
http://www.ociweb.com/mark/programming/ClojureSnake.html. Feedback is
welcomed!
I also started documenting some Clojure coding guidelines aimed at
making code more readable at
http://www.ociweb.com/mark/programming/ClojureCodingGuidelines.html
and would appreciate feedback on these. I expect there will be cases
where not following these is justified, which is why I refer to them
as guidelines instead of rules.
There's a big difference between the comments directed at someone
reading the code (possibly the author at a later date) and someone
wishing to use it. Function-level documentation strings serve only the
latter class of person.
Randall Schulz
Looking at this code the uppercase variables stands out.
This isn't idiomatic is it?
(def GRID_SIZE 10)
(def HEIGHT 600)
(def MARGIN 50)
I believe the idiom for global values like this is to place asterisks
around the name. Underscores (and CamelCase) should only be used when
required for Java interop:
(def *grid-size* 10)
(def *height* 600)
(def *margin* 50)
(def *x-index* 0)
(def *y-index* 1)
> I was following Java conventions of making constants all
> uppercase. Is there a convention for this in Clojure? Is there a way I
> could prevent them from being changed later?
I'm not aware of anyway to make a global constant. By using a Var as
you've done, the only way to change them is to use 'def' again (which
is a big hammer and very much discouraged inside regular functions) or
to use 'binding' to temporarily change their values within a
particular thread. I think what you have is sufficient for
communicating your intended meaning.
On a more general point, I'd personally recommend being wary of
over-investing in comments. This is not a radical recommendation, but
I'll bring up again anyway that thought that it's better to write code
in such a way that it explains itself than to add comments to code
that doesn't. Only when the former is insufficient should more
comments be added. Every line of comment is another line of code that
must be maintained, and worse than that it's a line that no compiler
or unit test is ever going to indicate as incorrect. When adding a
comment, I think it's appropriate to be sure that the maintenance cost
of the comment itself outweighs the maintenance cost of having no
comment (or a shorter or more general comment).
An example of this point -- Abhishek's original code used a hash
with:x and :y keys to indicate coordinates. I changed this to a
two-element vector only in pursuit of this particular puzzle's goals,
namely fewer lines of code. This is a very different goal from most
code, which should be maintainability, and also different from
tutorial code, which should be about teaching Clojure to someone who
doesn't already know it. For either of those latter goals, I would
contend Abhishek's solution was a better one -- using :x and :y help
indicate what's going on without global index names (like *x-index*)
or much extra commenting.
Another example -- if you find a particular use of #() to be too
confusing on its own, consider replacing it with a (fn ...), which
allows the naming of each argument as well as destructuring. This can
again improve readability without require more comments.
I think your expanded version of the snake program may be very
beneficial to some, though with a different purpose than the original
version, so thanks for providing it. Personally, I wouldn't want to
maintain a large codebase that was written using either style, as both
are a bit extreme in opposite ends of the verbosity scale. As I've
said elsewhere "golfing" is fun, if not broadly useful. Conversely,
tutorial-style code may be very useful in appropriate contexts, but is
hardly ever fun to write.
...and now this post is at an extreme of the verbosity scale. Sorry
all, I'll quit now before I get any further behind.
--Chouser
Thanks for the info. Stuart!
It's early enough in the life of Clojure that we haven't developed any
deeply held habits yet. I think it would be a good idea for you and
other Clojure committers to at least suggest the way you think things
should be done in code. If you think surrounding names of constants
with plus signs is a good way to identify them as such, I'll gladly
start doing that for the sake of consistency. I don't think it's a
good idea for all of us to simply do what feels good because that will
make it harder to read code written by others.
I think that's supposed to be + instead of *, at least Common Lisp
seems to use +.
> On a more general point, I'd personally recommend being wary of
> over-investing in comments. This is not a radical recommendation, but
> I'll bring up again anyway that thought that it's better to write code
> in such a way that it explains itself than to add comments to code
> that doesn't. Only when the former is insufficient should more
> comments be added.
I agree completely! In some programming languages, for example
Smalltalk, I feel like I can almost always write the code in a way
that doesn't require comments. However, I don't feel able to do that
as often in Clojure. I think it's because you can do so much with so
little code in Clojure. For example, here's some code that I don't
know how to rewrite in a way that I find self-explanatory:
(every?
#(<= (- (apple %) +grid-size+) (head %) (+ (apple %) +grid-size+))
[+x-index+ +y-index+]))
And here's another one:
(assoc snake :body
(cons
(vec (map #(+ (dir %) ((first body) %)) [+x-index+ +y-index+]))
(if grow body (butlast body)))))
Perhaps using your suggestion to go back and use a map with :x and :y
keys instead of a two-element vector to represent x/y coordinates
would help a little, but I'm still not sure the code would be
immediately obvious.
Maybe it's just a matter of time before I'm good enough at reading
Clojure code that I won't feel the need to add comments to these. Even
if that happens though, I'll still be concerned about the ability of
other developers that haven't yet reached that level of proficiency to
understand my code.
I meant * -- I don't know CL at all, but the *asterisk* form is used
frequently in clojure.core, while no +plus+ form ever appears. I also
was careful to refer to the global nature of the Vars, not anything
about const-ness.
--Chouser
Ah ... thanks for clarifying that! I've changed my code at
http://www.ociweb.com/mark/programming/ClojureSnake.html to follow
that convention.
(every? #(<= (- *grid-size*) % *grid-size*)
(map - apple head)))
and
(assoc snake :body
(cons
(map + dir (first body))
(if grow body (butlast body)))))
and
(defn paint [graphics [x y] color]
(.setColor graphics color)
(.fillRect graphics x y *grid-size* *grid-size*))
As a bonus, it works with any number of dimensions :-)
Christophe
The previous line needs to be (vec (map + dir (first body))).
> (if grow body (butlast body)))))
>
> and
>
> (defn paint [graphics [x y] color]
> (.setColor graphics color)
> (.fillRect graphics x y *grid-size* *grid-size*))
Excellent improvements! Thank you very much!
I've updated http://www.ociweb.com/mark/programming/ClojureSnake.html
to include these changes.
> As a bonus, it works with any number of dimensions :-)
Except for the fact that Java's Graphics object doesn't know how to
paint in 3D. ;-)
> Except for the fact that Java's Graphics object doesn't know how to
> paint in 3D. ;-)
Indeed. It works with n dimensions but you only see a 2D projection of
the play area :-(
Christophe
I thought the asterisk convention was for variables intended for
dynamic binding. It took me a minute to figure out where I got that
idea. "Programming Clojure" suggests it (without quite saying it) in
chapter 6, section 3.
"Vars intended for dynamic binding are sometimes called special vari-
ables. It is good style to name them with leading and trailing asterisks."
Obviously the book's a work in progress, but that does sound
reasonable. A special convention for variables whose values change (or
that my code's welcome to rebind) seems more useful to me than one for
"globals" (though I'm not sure I'd consider something like grid-size
for a given application a global). Based on ants.clj it appears Rich
doesn't feel there needs to be a special naming convention for that
sort of value.
What's your read on that, Chouser?