current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

I wrote the following code which transforms a nested data structure

(def board [[{:mine true}             {:warn 1 :explored true} {:explored true}]
           [{:warn 1 :explored true}  {:warn 1 :explored true} {}              ]
           [{}                        {}                       {:flag true }   ]])

into a printable form and prints it

(("[ ]" "[1]" "[E]") ("[1]" "[1]" "[ ]") ("[ ]" "[ ]" "[F]"))

The functions for the transformation are the following: 

(defn cell->icon 
  [cell]
  (letfn [(cell->str [v] (format "[%s]" v))]  
    (if (:explored cell) 
      (cond (:mine cell) (cell->str "M")
            (:warn cell) (cell->str (:warn cell))
            :else        (cell->str "E"))
      (cond (:flag cell) (cell->str "F")
            :else        (cell->str " ")))))

(defn board->icons
  [board]
  (map (partial map cell->icon) board))

So far so good (if you have any recommendations for a nicer approach though, do not hesitate to mention).

The function which I don't like though is the following: 

(defn print-board
  [board]
  (doall 
    (map println
       (map (partial clojure.string/join " ")
            (board->icons board)))))

I don't like to use println together with map since it is not a pure function (has side-effects)!? Maybe I am a bit too critical but I would be glad if somebody could advice me or give a hint how to do it in a nicer Clojure like way.

share|improve this question
up vote 3 down vote accepted

Separate impure IO from data transformations

I wrote the following code which transforms a nested data structure ... into a printable form

Not only does your code transform the data into a printable form, it also prints it.

To better isolate the side effects of printing, you should separate these two operations: first assemble a formatted string, then you can do whatever you want with it -- print it with a single call to println, save it, send it over the network, etc.

(defn format-board
  [board]
  (->> (board->icons board)
       (map (partial str/join " "))
       (str/join "\n")))

(def print-board
  (comp println format-board))

doseq

As a side note, instead of (doall (map println seq-to-print)) it is common to see

(doseq [line seq-to-print]
  (println line))

which is usually what you want, since

  • doseq does not hold onto the return value of println, whereas doall builds up an entire return sequence in memory (important when the sequence is big!) -- which is why your original print-board returns (nil nil nil) instead of nil; and
  • this makes it more visually clear that you are doing something with each item in the sequence, instead of generating a new sequence with map
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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