5
\$\begingroup\$

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))                                                            
            res (guess g word)]                                                                          
        (if (= word res)                                                                                 
          (do                                                                                            
            (.println out res)                                                                           
            (.println out "Correct!")                                                                    
            (.flush out))                                                                                
          (do                                                                                            
            (reset! the-guess g)                                                                         
            (recur conn)))))))                                                                           

(defn -main [& args]                                                                                     
  (println "Server running…")                                                                            
  (with-open [server (listen 3456)]                                                                      
    (loop []                                                                                             
      (let [client (.accept server)                                                                      
          in (BufferedReader.(InputStreamReader.(.getInputStream client)))                               
          out (PrintWriter. (.getOutputStream client))                                                   
          conn (ref {:in in :out out})]                                                                  
        (println "Client connected")                                                                     
        (doto (Thread. #(conn-handler conn)) (.start))                                                   
        (recur)))))
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

A bunch of random observations

  • 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))))))
\$\endgroup\$
1
  • \$\begingroup\$ I have some trouble getting it to work with the threading macro as you suggested. \$\endgroup\$
    – l3dx
    Commented Sep 27, 2013 at 15:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.