1

As I am learning clojure, I am trying to build a simple tic tac toe game, without ia. I started with a method in order to show a board, but it seems very ugly to me : i am using internal functions, in order to make them local to the show-board method, so that it can't be instantiated outside. Perhaps the reason of the bad looking.

Here is the function (which works as I want) :

(defn show-board [board]
    "Prints the board on the screen.
     Board must be a list of three lists of three elements with
     :cross for a cross and :circle for a circle, other value for nothing"
    (let [convert-elem (fn [elem] 
                            (cond
                                    (= elem :cross) "X"
                                    (= elem :circle) "O"
                                    :other "_"))
          convert-line (fn [elems-line]
                            (reduce str (map convert-elem elems-line)))]
         (doseq [line board]
            (println (convert-line line)))))

Here is a use case :

(show-board (list (list :cross :circle :none) (list :none :circle :none) (list :cross :none :none)))

Sorry for the ugly code, this is because I come from Java, and I am starting in Clojure. (I think I will really get benefits from learning Clojure, and make games with it, so I can't just leave it).

Another reason why I want to simplify it is for code maintenance and readability.

Thanks in advance

loloof64
  • 5,252
  • 12
  • 41
  • 78

3 Answers3

2

Using internal functions is perfectly fine, though in this case using letfn would perhaps look better. Additionally, convert-elem can be simplified with case and convert-line should use apply rather than reduce for reasons I explained in my answer to the Clojure: reduce vs. apply SO question (in short, with apply a single StringBuilder is used and the resulting process is linear; with reduce, each step involves allocating a new StringBuilder and the process is quadratic; this doesn't make much difference is a small case like this, but it is nonetheless better style to use the correct approach).

Here's the modified function:

(defn show-board [board]
  "Prints the board on the screen.
   Board must be a list of three lists of three elements with
   :cross for a cross and :circle for a circle, other value for
   nothing."
  (letfn [(convert-elem [elem] 
            (case elem
              :cross "X"
              :circle "O"
              "_"))
          (convert-line [elems-line]
            (apply str (map convert-elem elems-line)))]
    (doseq [line board]
      (println (convert-line line)))))

As a side note, this function actually takes an arbitrary seqable of seqables, not necessarily a list of lists. A more usual concrete type choice in Clojure would be to use vectors:

user> (show-board [[:cross :circle :none]
                   [:none :circle :none]
                   [:cross :none :none]])
XO_
_O_
X__
nil
Community
  • 1
  • 1
Michał Marczyk
  • 83,634
  • 13
  • 201
  • 212
  • 1
    `convert-element` function can be replaced with a map `{:cross \X :circle \O :none \_}`. IMHO it is more concise. – Mikita Belahlazau Aug 21 '13 at 21:47
  • 1
    @NikitaBeloglazov Right, as long as the spec (as expressed in the docstring) is changed to require `:none` as the "empty" value. Or we could use `{:cross \X :circle \O}` and look up the result with `(get convert-elem elem \_)`. – Michał Marczyk Aug 21 '13 at 21:50
  • Yes. I can add that but you can do it without `get`: `(convert-elem elem \_)`. – Mikita Belahlazau Aug 21 '13 at 21:52
  • Wow :) Thank you very much. Didn't know letfn : will use it know when appropriate. Did not know for the complexity of reduce when dealing with strings. Thank also for the comments for the "\" reader. – loloof64 Aug 21 '13 at 21:55
  • 1
    @NikitaBeloglazov Indeed. Thanks for the comments! @LaurentBERNABE To add one more remark to the answer, while I think doing everything in one function is fine in this case (as it's really simple), if this logic were to become more involved it would probably make sense to pull things apart -- put the internal functions at top level (possibly marking them private), make a separate function construct a representation of the board useful for printing (a string or seq of strings), then make `show-board` call that function. That way it would all stay clear and testable despite the extra complexity. – Michał Marczyk Aug 21 '13 at 21:58
  • I had a doubt for whether to describe the board as list or as a vector, but you're right, vectors are better (the first value can't be interpreted as a function). – loloof64 Aug 21 '13 at 21:59
  • @MichałMarczyk Ok, I will search for the private option and use it in my code. Thanks again. – loloof64 Aug 21 '13 at 22:02
  • Finally, here is my modifications, with all your advices : http://pastebin.com/vNNMV80R – loloof64 Aug 21 '13 at 22:33
2
(defn show-board
  [board]
  (let [convert (fn [el] (get {:cross \X :circle \O} el \_))]
    (doseq [line board]
      (doseq [el line]
        (print (convert el)))
      (println))))
noisesmith
  • 20,076
  • 2
  • 41
  • 49
  • Apart from its beauty, this also reminds me the nested for of Java-like languages (it is simpler than our previous attempt). – loloof64 Aug 21 '13 at 23:07
2

What about simply letting the underlying writer to buffer the output rather than creating intermediate strings (even if you create them through a StringBuilder)?

=> (defn show-board [board]
     (doseq [line board]
       (doseq [item line]
         (print ({:cross \X :circle \O} item \_)))
       (newline)))

And if you want to get a string rather than print them out, just use with-out-str.

cgrand
  • 7,939
  • 28
  • 32