patternMinor
Hangman - my first Clojure code
Viewed 0 times
codefirstclojurehangman
Problem
This is my first attempt at Clojure! Apart from all game related issues, how is my code? Is there anything I can do to make it more idiomatic?
```
(ns hangman.core
(:import (java.net ServerSocket Socket SocketException)
(java.io PrintWriter InputStreamReader BufferedReader))
(:gen-class))
(defn guess [guess word]
(apply str (map #(let [x (str %)] (if (.contains guess x) x "*")) word)))
(defn listen [port]
(new ServerSocket port))
(def word "clojure")
(defn conn-handler [conn]
(let [in (:in @conn)
out (:out @conn)]
(.println out "Welcome to this simple hangman game.")
(def the-guess (atom ""))
(loop [conn conn]
(.println out (str "Guess the word: " (guess @the-guess word)))
(.flush out)
(let [g (str @the-guess (.readLine in))
```
(ns hangman.core
(:import (java.net ServerSocket Socket SocketException)
(java.io PrintWriter InputStreamReader BufferedReader))
(:gen-class))
(defn guess [guess word]
(apply str (map #(let [x (str %)] (if (.contains guess x) x "*")) word)))
(defn listen [port]
(new ServerSocket port))
(def word "clojure")
(defn conn-handler [conn]
(let [in (:in @conn)
out (:out @conn)]
(.println out "Welcome to this simple hangman game.")
(def the-guess (atom ""))
(loop [conn conn]
(.println out (str "Guess the word: " (guess @the-guess word)))
(.flush out)
(let [g (str @the-guess (.readLine in))
Solution
A bunch of random observations
-
avoid
-
there's no need to use a ref for the connection, as it never changes after creation. Prefer a simple map, which also enables you to use plain destructuring instead of using
-
instead of using interop to interact with the in/out streams you could use a more idiomatic
-
you loop over the
-
if you follow the
The following code contains the revised
-
avoid
def as a non-top level form e.g. in (def the-guess (atom "")), prefer let bindings-
there's no need to use a ref for the connection, as it never changes after creation. Prefer a simple map, which also enables you to use plain destructuring instead of using
let to extract its components:(defn conn-handler [{:keys [in out] :as conn}] ...)
...
conn {:in in :out out}]
(println "Client connected")
(doto (Thread. #(conn-handler conn)) (.start))-
instead of using interop to interact with the in/out streams you could use a more idiomatic
(binding [out out in in] (println ...) (read-line))-
you loop over the
conn, but you always recur over the very same object. You can remove conn from the loop/recur forms-
if you follow the
binding approach as described above, but within the anonymous function that you use to initialize the thread, you could completely remove the conn name from the whole code:(defn conn-handler [] (println "...") ...)
(doto (Thread. #(binding [*in* in *out* out] (conn-handler)) (.start))- there's no need to use an atom for the guess, just use plain recursion:
(loop [the-guess ""]
...
(recur g))The following code contains the revised
conn-handler and -main functions, including all the suggestions above, plus the cosmetic change of using the threading macro -> instead of the doto form, which seems a bit clearer:(defn conn-handler []
(println "Welcome to this simple hangman game.")
(loop [the-guess ""]
(println (str "Guess the word: " (guess the-guess word)))
(flush)
(let [g (str the-guess (readLine))
res (guess g word)]
(if (= word res)
(do
(println res)
(println "Correct!")
(flush))
(do
(recur g))))))
(defn -main [& args]
(println "Server running…")
(with-open [server (listen 3456)]
(while true
(let [client (.accept server)
in (BufferedReader.(InputStreamReader.(.getInputStream client)))
out (PrintWriter. (.getOutputStream client))]
(println "Client connected")
(-> #(binding [*in* in
*out* out]
conn-handler)
(Thread.)
(.start))))))Code Snippets
(defn conn-handler [{:keys [in out] :as conn}] ...)
...
conn {:in in :out out}]
(println "Client connected")
(doto (Thread. #(conn-handler conn)) (.start))(defn conn-handler [] (println "...") ...)
(doto (Thread. #(binding [*in* in *out* out] (conn-handler)) (.start))(loop [the-guess ""]
...
(recur g))(defn conn-handler []
(println "Welcome to this simple hangman game.")
(loop [the-guess ""]
(println (str "Guess the word: " (guess the-guess word)))
(flush)
(let [g (str the-guess (readLine))
res (guess g word)]
(if (= word res)
(do
(println res)
(println "Correct!")
(flush))
(do
(recur g))))))
(defn -main [& args]
(println "Server running…")
(with-open [server (listen 3456)]
(while true
(let [client (.accept server)
in (BufferedReader.(InputStreamReader.(.getInputStream client)))
out (PrintWriter. (.getOutputStream client))]
(println "Client connected")
(-> #(binding [*in* in
*out* out]
conn-handler)
(Thread.)
(.start))))))Context
StackExchange Code Review Q#31752, answer score: 3
Revisions (0)
No revisions yet.