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!