Improving my first Clojure program

Question

After a few weekends exploring Clojure I came up with this program. It allows you to move a little rectangle in a window. Here's the code:

(import java.awt.Color)
(import java.awt.Dimension)
(import java.awt.event.KeyListener)
(import javax.swing.JFrame)
(import javax.swing.JPanel)

(def x (ref 0))
(def y (ref 0))

(def panel
  (proxy [JPanel KeyListener] []
    (getPreferredSize [] (Dimension. 100 100))
    (keyPressed [e]
      (let [keyCode (.getKeyCode e)]
        (if (== 37 keyCode) (dosync (alter x dec))
        (if (== 38 keyCode) (dosync (alter y dec))
        (if (== 39 keyCode) (dosync (alter x inc))
        (if (== 40 keyCode) (dosync (alter y inc))
                            (println keyCode)))))))
    (keyReleased [e])
    (keyTyped [e])))

(doto panel
  (.setFocusable true)
  (.addKeyListener panel))

(def frame (JFrame. "Test"))
(doto frame
    (.add panel)
    (.pack)
    (.setDefaultCloseOperation JFrame/EXIT_ON_CLOSE)
    (.setVisible true))

(defn drawRectangle [p]
  (doto (.getGraphics p)  
    (.setColor (java.awt.Color/WHITE))
    (.fillRect 0 0 100 100)
    (.setColor (java.awt.Color/BLUE))
    (.fillRect (* 10 (deref x)) (* 10 (deref y)) 10 10)))

(loop []
  (drawRectangle panel)
  (Thread/sleep 10)
  (recur))

Despite being an experienced C++ programmer I found it very challenging to write even a simple application in a language that uses a radically different style than what I'm used to.

On top of that, this code probably sucks. I suspect the globalness of the various values is a bad thing. It's also not clear to me if it's appropriate to use references here for the x and y values.

Any hints for improving this code are welcome.

Answer 1

Those ifs in keyPressed can be replaced with a single case. Also, the dosync can be moved outside to wrap the case. In fact, alter can be moved out too, so that if you e.g. decide to change it to commute, there's just the one place to make the change. The result:

(def panel
  (proxy [JPanel KeyListener] []
    (getPreferredSize [] (Dimension. 100 100))
    (keyPressed [e]
      (let [keyCode (.getKeyCode e)]
        (dosync
         (apply alter
           (case keyCode
             37 [x dec]
             38 [y dec]
             39 [x inc]
             40 [y inc])))
        (println keyCode)))
    (keyReleased [e])
    (keyTyped [e])))

You could also rewrite the imports more concisely:

(import [java.awt Color Dimension event.ActionListener])
(import [javax.swing JFrame JPanel])

-- whether you'd want to is a matter of style.

I would rename drawRectangle to draw-rectangle (that's the idiomatic style for function names in Clojure) and, more to the point, rewrite it as a pure function accepting the coordinates as explicit arguments. Then you could write a small wrapper around that to use your Refs, if indeed your design would benefit from the use of Refs. (Hard to say without knowing how you might want to use & modify them etc.)

Prefer while to (loop [] ... (recur)) (see (doc while) and (clojure.contrib.repl-utils/source while)).

Also -- and this is important -- don't put anything except definitions at top level. That's because top-level forms are actually executed when the code is compiled (try loading a library with a (println :foo) at top level). That infinite loop should be wrapped inside a function; the standard name for a "main" function in Clojure is -main; same goes for panel and frame. This doesn't apply when playing around at the REPL, of course, but it's an important gotcha to know about.

Incidentally, (doto foo ...) returns foo, so you can just write (doto (proxy ...) (.setFocusable true) ...).

Otherwise, I'd say the code is fine. Normally you'd want to put it in a namespace; then all the imports would go in the ns form.

HTH

Answer 2

Although not exactly Clojure advice - consider using KeyAdapter instead of KeyListener. That way you won't have to provide empty implementations for keyReleased and keyTyped. It's generally a good rule to use adapter classes when you're not needing all of the methods of a Listener interface.

Answer 3

Edit, while writing the below post in a hurry I forgot to say you must not put parens around constants eg java.awt.Color/WHITE.

In addition to Michał's comments:

• when you use an infinite loop, condition it with an atom (eg @switch) thus you'll be able to stop it (unless the loops runs on the repl thread -- so lauch it in a separat thread with "future")

• you use refs, so it means the values of x and y are intended to be coordinated (in your code they are not: each update is independent); hence in drawRectangle you should wrap the reads in a dosync to be sure that you get a consistent view -- again in your actual code it doesn't matter because x and y are always updated independently.

hth,

(shameless plug: http://conj-labs.eu/)