CODEWITHSUNDEEP
clojure
Xi Han
Xi Han

Reputation: 323

Unexpected behavior of function

I write the following function to merge s2 into s1, and any element whose "id" value is same as any element in s1 will not be merged.

(defn into-unique-id 
  [s1, [x2 & xs2]]
  (if x2 (if (empty (filter (fn [x] (= (get x "id") (get x2 "id"))) s1)) (into-unique-id (conj s1 x2) xs2) (into-unique-id s1 xs2)) 
         s1))

I tried the following in REPL.

gridizela.core=> (def s1 [{"id" "1"} {"id" "2"}])
#'gridizela.core/s1
gridizela.core=> (into-unique-id s1 [{"id" "1"}])
[{"id" "1"} {"id" "2"} {"id" "1"}]

As you can see, the result was not as I expected, in which the last element should not be added.

Any help would be appreciated.

Upvotes: 0

Views: 55

Answers (1)

noisesmith
noisesmith

Reputation: 20194

I had to do a bit of refactoring in order to fix your problem, I am going to include each version of the code to show the steps needed to get the ideal result.

;; original
(defn into-unique-id 
  [s1, [x2 & xs2]]
  (if x2 (if (empty (filter (fn [x] (= (get x "id") (get x2 "id"))) s1)) (into-unique-id (conj s1 x2) xs2) (into-unique-id s1 xs2)) 
         s1))

The original code is unusually laid out, in a way that makes it hard for me to read, and hard for me to understand. So my first step was to apply normal indentation.

;; indentation
(defn into-unique-id-2
  [s1, [x2 & xs2]]
  (if x2
    (if (empty (filter (fn [x] (= (get x "id")
                                  (get x2 "id")))
                       s1))
      (into-unique-id (conj s1 x2) xs2)
      (into-unique-id s1 xs2)) 
    s1))

I still didn't quite get the code when I got to this point, but I saw some small changes that would make it easier to read. cond is almost always the thing you want instead of nested if conditionals. I use ,, as whitespace to differentiate the result clauses from the condition clauses in cond.

;; simplification
(defn into-unique-id-3
  [s1, [x2 & xs2]]
  (cond (not x2)
        ,, s1
        (empty (filter (fn [x] (= (get x "id")
                                  (get x2 "id")))
                       s1))
        ,, (into-unique-id (conj s1 x2) xs2)
        :else
        (into-unique-id s1 xs2)))

At this point I finally saw the error: (empty x) will return truthy for any input that is not nil, even an empty collection. What we actually want here is the deceptively similarly named (but very different) empty? function.

;; fix -- empty is always truthy here
(defn into-unique-id-4
  [s1, [x2 & xs2]]
  (cond (not x2)
        ,, s1
        (empty? (filter (fn [x] (= (get x "id")
                                   (get x2 "id")))
                        s1))
        ,, (into-unique-id (conj s1 x2) xs2)
        :else
        (into-unique-id s1 xs2)))

next I saw that instead of filter / empty? we can use the built in some.

;; simplification -- we have idiomatic functions for this
(defn into-unique-id-5
  [s1, [x2 & xs2]]
  (cond (not x2)
        ,, s1
        (some (fn [x] (= (get x "id")
                         (get x2 "id")))
                        s1)
        ,, (into-unique-id s1 xs2)
        :else
        ,, (into-unique-id (conj s1 x2) xs2)))

Early on I noticed this was effectively a reduce done by hand, and so as a last step I'll show the function as a reduce.

;; structural simplification - we have idiomatic higher order functions
;; that we can use instead of recursion
(defn into-unique-id-6
  [s1, coll]
  (reduce 
   (fn [acc el]
     (if (some (fn [x]
                 (= (get x "id")
                    (get el "id")))
               acc)
       acc
       (conj acc el)))
   s1
   coll))

Upvotes: 6

Related Questions