2
\$\begingroup\$

Just working through the questions on 4Clojure as I learn the language. This is my solution for #69. Would appreciate feedback as this one gave me a little trouble.

(fn merge-with2 [f m & ms]
   (let [update-or-add (fn update-or-add [m1 k f arg] 
                          (let [x (get m1 k)]
                            (if (nil? x)
                              (conj m1 {k arg})
                              (update-in m1 [k] f arg))))]
   (loop [m1 m m2 (first ms) ms ms
          k (keys m2)]
     (if-not (map? m2)
       m1
       (recur (reduce #(update-or-add %1 %2 f (get m2 %2)) m1 k)
              (first (next ms))
              (next ms)
              (keys (first (next ms))))))))

I suspect that it would be possible to replace the loop/recur with a nested reduce, but I'm not quite sure how to implement it.

\$\endgroup\$
2
\$\begingroup\$

Here are my thoughts:

letfn instead of let

You could use letfn instead of let, which would be a little more concise:

(letfn [(update-or-add [m1 k f arg]
          ...

conj vs. update-in

conj and update-in are very similar functions in that they both take a map and "update" the value for a key that may or may not exist. Because of this, they are sometimes exchangeable. update-in is fine here, but I think using conj for both scenarios could make it easier to see at a glance what your function is doing. (See the next example.)

if-let

The (let [x (get m1 k)] (if (nil? x) ... part of your code is a good example of where if-let comes in handy.

(if-let [x (m1 k)]
  (conj m1 {k (f x arg)})
  (conj m1 {k arg}))

Which you could even simplify further by moving the if-let inside of a single conj function call:

(conj m1 {k (if-let [x (m1 k)] (f x arg) arg)})

As an aside, I tend to prefer assoc, although it's really just as good as conj:

(assoc m1 k (if-let [x (m1 k)] (f x arg) arg))

Reducers work best as functions taking 2 arguments

The first thing I noticed about your loop is that its arguments can be simplified:

(loop [m1 m, ms ms]
  ...

Essentially what you're doing is starting with m1 and reducing update-or-add over each key-value pair in each map in ms. The tricky thing here is that, as you pointed out, this is a nested reducing operation, in that each map in ms can have multiple key-value entries. You have the right general idea in that you have a reduce within a loop, but a better/simpler way to do this would be to make your update-or-add function take the two arguments that a reducing function expects -- an accumulator and the next item in the collection being reduced over. Once you have update-or-add working that way, you can do the same thing with your loop and at that point you'll be able to represent your solution as a simple nested reduce.

Let's start with update-or-add. This function takes a working "map in progress" and a key-value pair and either adds it to the map if that key isn't already there, or it updates the value to be the result of calling f with the value already there and the new value as arguments. Note that we don't need to make f an argument because it's already available within the context of the entire function merge-with2. You can also use destructuring to easily access the key and the value of the key-value pair.

(letfn [(update-or-add [m1 [k v]]
          (assoc m1 k (if-let [x (m1 k)] 
                        (f x v) 
                        v)))]
  ...

Now you can use update-or-add "as-is" within a reduce.

You can do something similar with your main loop. To simplify things, let's make it its own function, and call it something like merge-with-one. This function will take one of the maps supplied to merge-with2 and reduce update-or-add over its key-value pairs. Defining this function is actually super easy, because it's just a call to reduce. So, here is my suggested refactoring of your code:

(fn merge-with2 [f m & ms]
  (letfn [(update-or-add [m1 [k v]]
            (assoc m1 k (if-let [x (m1 k)] 
                          (f x v) 
                          v)))
          (merge-with-one [m1 m2]
            (reduce update-or-add m1 m2))]
    (reduce merge-with-one m ms)))

It turns out that this is actually pretty close to the actual implementation of merge-with in clojure.core.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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