On Sun, Dec 28, 2008 at 9:15 PM, Rich Hickey <richhic...@gmail.com> wrote:
>
> On Dec 28, 8:13 pm, "Mark Volkmann" <r.mark.volkm...@gmail.com> wrote:
>> This is related to the thread titled "Proxying in Clojure". I've spent
>> a lot of time studying the code in this "snake" example and have some
>> observations I'd like to share.
>>
>> 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
>> athttp://pastie.org/348031where 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 athttp://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.
>
> I'll not argue for making code harder to read, but I have to object to
> most of your example.
>
> Making something 4x longer does not make it easier to read.
>
> Redundant comments are useless.
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.
--
R. Mark Volkmann
Object Computing, Inc.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Clojure" group.
To post to this group, send email to clojure@googlegroups.com
To unsubscribe from this group, send email to
clojure+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
-~----------~----~----~----~------~----~------~--~---
