2
\$\begingroup\$

This is the essence of chapter 3 of "Clojure for the Brave and True". This program is about completing the list asym-hobbit-body-parts by adding a given number of elements called "0-<name>" "1-<name>" and so on for every element in asym-hobbit-body-parts that starts with "left" and subsequently choosing a random body part to hit. This contains code from the book which I rewrote from memory after finishing chapter 3. (And I mixed it with what I learned on the official homepage)

I am very curious what a clean version of this would look like.

(ns clojure-noob.core
  (:gen-class)
  (:require [clojure.string :as str] ))


(def asym-hobbit-body-parts [{:name "head" :size 3}
                             {:name "left-eye" :size 1}
                             {:name "left-ear" :size 1}
                             {:name "mouth" :size 1}
                             {:name "nose" :size 1}
                             {:name "neck" :size 2}
                             {:name "left-shoulder" :size 3}
                             {:name "left-upper-arm" :size 3}
                             {:name "chest" :size 10}
                             {:name "back" :size 10}
                             {:name "left-forearm" :size 3}
                             {:name "abdomen" :size 6}
                             {:name "left-kidney" :size 1}
                             {:name "left-hand" :size 2}
                             {:name "left-knee" :size 2}
                             {:name "left-thigh" :size 4}
                             {:name "left-lower-leg" :size 3}
                             {:name "left-achilles" :size 1}
                             {:name "left-foot" :size 2}])


(defn make-sym-parts [asym-set num]
  (reduce (fn [sink, {:keys [name size] :as body-part}]
           (if (str/starts-with? name "left-")
             (into sink (apply vector body-part 
                         (for [i (range num)]
                           {:name (str/replace name #"^left" (str i))
                            :size size})))
             (conj sink body-part)))
           []
           asym-set))


(defn rand-part [parts]
  (let [size-sum (reduce + (map :size parts))
        thresh (rand size-sum)]
    (loop [[current & remaining] parts
           sum (:size current)]
      (if (> sum thresh)
        (:name current)
        (recur remaining (+ sum (:size (first remaining))))))))


(defn -main
  [& arg]
  (println (rand-part (make-sym-parts asym-hobbit-body-parts 3))))
\$\endgroup\$
2
  • \$\begingroup\$ I'm sorry, I adjusted the title. But I think you pasted the wrong link ^^ \$\endgroup\$
    – Uzaku
    Commented Jul 30, 2019 at 12:54
  • \$\begingroup\$ Ah, seems the "meta" disappeared from the URL in in my template file. Apologies, the correct URL is CodeReview.Meta.StackExchange.com/q/2436 . \$\endgroup\$
    – BCdotWEB
    Commented Jul 30, 2019 at 12:57

1 Answer 1

0
\$\begingroup\$

You actually already fixed up the main thing that I was going to comment on: your previous use of def over let. Just to emphasize on why it's important to choose one over the other though:

(defn f []
  (def my-var 5))

(f)

(println my-var)
5  ; It leaked the state of the function and polluted the namespace

Beyond that, this isn't bad code. As I mentioned on SO, I'm a little rusty, so hopefully someone else will go a little more in-depth. I'll mention what I see here though:

This might be a little over-engineering, but I don't like how the asym-hobbit-body-parts definition is repeating the keys :name and :size over and over. This allows typos to slip in, and makes more work for you if you want to add more parts. I'd make a "constructor" that automatically creates the maps from the data:

(defn new-part [part-name part-size]
  {:name part-name, :size part-size})

(def asym-hobbit-body-parts [(new-part "head" 3)
                             (new-part "left-eye" 1)
                             (new-part "left-ear" 1)])

I like this because now there's a function that definitively says what a "part" is defined as, and if you ever want to change what keywords you use, you only need to change new-part now.

This still has duplication though, and could be reduced down with a helper that auto-wraps vector "tuples":

(defn new-parts [& name-size-pairs]
  (mapv #(apply new-part %) name-size-pairs))
  ; or (mapv (partial apply new-part) name-size-pairs))  ; Partially apply "apply"

(def asym-hobbit-body-parts3 (new-parts
                               ["head" 3]
                               ["left-eye" 1]
                               ["left-ear" 1]))

Or, if you want to get rid of the need for the wrapping vectors, you can use partition to cut a list of attributes into 2s:

(defn new-parts [& name-size-pairs]
  (->> name-size-pairs
       (partition 2)  ; Cut the list into pairs of 2
       (mapv #(apply new-part %))))

(def asym-hobbit-body-parts3 (new-parts
                               "head" 3,
                               "left-eye" 1,
                               "left-ear" 1))

It really depends on how you want it to read.

In case you aren't familiar with them, I highly recommend practicing use of -> and ->>. They seem a little complicated at first, but once you understand them, they have the potential to make your code much neater. They allow your code to read more easily as a series of transformations.


I think the reduction in make-sym-parts is too big. When I have a call to reduce with a large reducing function like that, I try to move it out into its own standalone function. That gives that code a name to make it clearer what it's doing, and makes the call to reduce easier to understand. I decided just to make it a local function using letfn. You could also make it a global function using defn; but I don't think that's necessary.

(defn make-sym-parts [asym-set num]
  (letfn [(transform-lefts [sink {:keys [name] :as body-part}]
            (if (str/starts-with? name "left-")
              (into sink (apply vector body-part
                                (for [i (range num)]
                                  (update body-part :name
                                          #(str/replace % #"^left" (str i))))))

              (conj sink body-part)))]

    (reduce transform-lefts [] asym-set)))

I also altered what you're doing in the for comprehension. You had a literal {:name ..., :size size} in there. This isn't good practice though. Pretend in the future you decide to add a new attribute to body parts. Maybe :quality or something. What happens if you forget to update make-sym-parts? Because you're reconstructing the map from scratch, the new body part will be discarded! update is like assoc, but it "updates" a previous value using a function instead of overwriting it like assoc does. The previous value is passed into the function, and the new value becomes whatever the function returns.

This could still be improved a bit by making use of mapcat. Right now, you're using reduce instead of map because in some cases, you need to add multiple entries per existing element. mapcat (basically short for "map then concatenate") can help here. It will automatically flatten the resulting list. This means you can get rid of the sink accumulator:

(defn make-sym-parts [asym-set num]
  (letfn [(transform-lefts [{:keys [name] :as body-part}]
            (if (str/starts-with? name "left-")
              [body-part
               (for [i (range num)]
                 (update body-part :name
                         #(str/replace % #"^left" (str i))))]

              [body-part]))]  ; Note how this requires a wrapping so it can be flattened

    (mapcat transform-lefts asym-set)))  ; Lazy!

The major drawback here is I wouldn't be surprised if this is slower. That likely isn't an issue though.


I was going to suggest you change rand-part to use reduce (I had a whole thing written up and everything!), then I realized that it requires knowledge of the next part in the list. It is still possible to use reduce, but it would require doing something messy like partitioning the list of parts into pairs, then reducing over the pairs. I think what you have here is likely neater.

I will mention though, just in case you didn't know, reduce allows for an early exit using reduced. If you didn't need to use remaining in the loop, that would allow your loop here to be written using reduce.

The only real two noteworthy things in rand-part are

  • I would use :keys here to deconstruct current. You're using explicit key-access a lot which adds some bulk.

  • (reduce + can also be written as (apply +. If you look at the definition of +, it and most other binary operators have a var-arg overload to delegates to a reduction. It's not a big deal either way. From what I've seen, (apply + is generally regarded as more idiomatic, but not by much.

Something like:

(defn rand-part [parts]
  (let [size-sum (apply + (map :size parts))
        thresh (rand size-sum)]
    (loop [[{:keys [name size]} & remaining] parts
           sum size]
      (if (> sum thresh)
        name
        (recur remaining
               (+ sum (:size (first remaining))))))))

Good luck and welcome to Clojure!

\$\endgroup\$
2
  • \$\begingroup\$ Wow this is a lot. Thank you for your effort. I think I'll be back in a few weeks with a real project instead of a toy example :) It is awesome that you took the time to explain all this. Sadly I can't even upvote your answer since I dont have 15 reputation yet \$\endgroup\$
    – Uzaku
    Commented Jul 30, 2019 at 19:25
  • \$\begingroup\$ @Uzaku No problem, I enjoy reviewing code. I'm looking forward to seeing what you come up with in the next couple weeks. \$\endgroup\$ Commented Jul 30, 2019 at 19:46

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