Fork me on GitHub
#code-reviews
<
2020-08-27
>
stopa15:08:28

Hey teams, one beginner code review:

``````(defn print-positions [board-size positions]
(println "---------")
(->> (range board-size)
(map (fn [x]
(map (fn [y]
(if (positions [x y])
"X"
"_"))
(range board-size))))
(run! println)))``````
Effectively, given a "board-size", and a set of positions, print a nice representation with those positions ^ Would you write this differently?

jaihindhreddy18:08:29

Instead of using `range` and processing indices, I'd make the matrix first, using `repeat`, then fill in the positions with `reduce` and `assoc-in`, and then make the right strings for each line using `map` and finally `run!` `println` to print it:

``````(defn print-positions [n positions]
(let [matrix (vec (repeat n (vec (repeat n \_))))
filled (reduce #(assoc-in % %2 \X) matrix positions)]
(->> (map #(str/join \space %) filled)
(run! println))))``````
And an example call:
``````user=> (print-positions 10 #{[0 0] [1 1] [3 3]})
X _ _ _ _ _ _ _ _ _
_ X _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ X _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _
nil
user=> ``````

❤️ 3
stopa15:08:01

(Also an aside: was very confused by print. Sometimes if I wrote `print` , it would not show up in the repl. Also I couldn't seem to get print to "print out" strings with newlines in them, represented at actual new lines. Will need to look deeper, but if you have thoughts / resources on how to best print in clj, lmk : })

TMac16:08:55

TBH, I feel like wrapping inherently side-effecting code in functional constructs can get a little cumbersome. I’d just go imperative and use a `doseq`:

``````(defn print-positions2
[occupied? board-size]
(println (str/join (repeat 10 "-")))
(doseq [y (range board-size)
x (range board-size)]
(print (if (occupied? [x y]) "X" "_"))
(when (= x (dec board-size))
(println))))``````
(worth noting that someone could probably one-line this with `cl-format`, but this is the Clojurians Slack, not the Common Lispers one)

stopa18:08:35

Oo very nice ideas. Will also look deeper on cl-format! Thanks @jaihindhreddy @timgilbert

⁉️ 3
phronmophobic18:08:25

I think splitting it into two `doseq`s makes it slightly more readable, but I agree with @tsmacdonald that `doseq` is a better fit for side-effecting code:

``````(defn print-positions2
[occupied? board-size]
(println (str/join (repeat 10 "-")))
(doseq [y (range board-size)]
(doseq [x (range board-size)]
(print (if (occupied? [x y]) "X" "_")))
(println)))``````

❤️ 3
stopa19:08:34

Nice, thanks team!

stopa19:08:18

Okay, one more -- this time a bit more involved: This is the code to encode a message using a hoffman tree (@joeaverbukh and I are going through SICP together xD) 

``````(defn encode [tree message]
(letfn [(encode-symbol-helper [symbol path branch]
(cond
(= [symbol] (symbols branch))
path

:else
(let [left (left-branch branch)
right (right-branch branch)]
(or
(and left (encode-symbol-helper symbol (conj path 0) left))
(and right (encode-symbol-helper symbol (conj path 1) right))))))
(encode-symbol [symbol]
(if-let [res (encode-symbol-helper symbol [] tree)]
res
(throw (Exception. (format "Uh oh, couldn't find symbol = %s" symbol)))))]
(mapcat encode-symbol message)))

(comment
(def sample-tree (make-code-tree (make-leaf 'A 4)
(make-code-tree (make-leaf 'B 2) (make-code-tree
(make-leaf 'D 1)
(make-leaf 'C 1)))))
(def sample-bits '(0 1 1 0 0 1 0 1 0 1 1 1 0))
(def sample-message '(A D A B B C A))
(= (encode sample-tree sample-message) sample-bits))``````
Idea: is, for each symbol, we dfs over the tree, and return the path which leads to the correct leaf node. We throw if any errors If can be written even more clojure-style or consicely lmk : }  full code https://github.com/nezaj/clj-sicp/blob/master/src/chp2.clj#L230-L253

TMac19:08:44

I haven’t had enough coffee to talk about the algorithm, but the `cond` can be replaced by an `if` and `encode-symbol-helper` can be replaced by providing multiple arities to `encode` (something like this:)

``````(defn encode
([tree message]
(letfn [(encode-symbol …]
(mapcat encode-symbol message)))
([symbol path branch]
(if (= [symbol] …)))``````

❤️ 3
stopa20:08:11

Great ideas! And nice profile pic @tsmacdonald! : )

seancorfield21:08:07

I would suggest using `sym` as the local binding so you don't shadow `symbol` in `clojure.core` -- I found the code hard to read because of that (I kept thinking you were referring to `clojure.core/symbol` in various calls).

❤️ 3
stopa21:08:38

Oi great point!