Fork me on GitHub
#code-reviews
<
2020-07-23
>
drewverlee20:07:40

Code puzzle and a review rolled into one. I need a way to translate essential the get-in list args into a datomic where clause.

[] -> [[]]
[:a] -> [[?r0 :a ?r1]]
[:a :b] -> [[?r0 :a ?r1] [?r1 :b ?r2]]

fn: expression, in-binding, find-binding -> where-clause

where expression is like the get-in arg [:a :b ... ]
Additionally the first and last bindings (r?0 and ?r2) are passed into the function. This is my first attempt that works:
(defn expression->where-clause
  [attrs in-binding find-binding]
  (let [->b #(symbol (str "?" %))
        f_attrs (first attrs)
        l_attrs (last attrs)]
    (cond
      (= 0 (count attrs)) []
      (= 1 (count attrs)) (into [find-binding] (conj attrs in-binding))
      (= 2 (count attrs)) [[find-binding f_attrs (->b (name f_attrs))]
                           [(->b (name f_attrs)) l_attrs in-binding]]
      :else
      (let [b    (butlast (rest attrs))
            body (:coll (reduce
                         (fn [{:keys [coll c]} n]
                           {:coll (conj coll (->b c) n (->b (inc c)))
                            :c    (inc c)})
                         {:coll []
                          :c    0}
                         b))
            fb   (first body)
            lb   (last body)]
        (partition 3
                   (into [find-binding f_attrs fb]
                         (conj body lb l_attrs in-binding)))))))
I feel like interleave should be at play here but when i wasn't sure it fit so i started with reduce to get something more basic.

phronmophobic21:07:02

just some initial thoughts: • 0 and 1 numbers of attributes don't seem like special cases. making them special cases in the code just makes it more verbose. the only reason to do that is if performance is especially important • why f_attrs instead of f-attrs? • b is only used once and its name is confusing. i would inline it into the reduce call • you use c to represent a number and the n to represent a keyword. if you're going to use single character names, use i ,`num` or n for the number (not c). use k to for the keyword, not n. • I think coll should be a vector of subqueries. ie.

(conj coll [ (->b c) n (->b (inc c))])
this would get rid of the partition at the end, but more importantly, it shows the structure of your intent more clearly • I would try to find a better name than coll. maybe query, stmts, or exprs? • if fb and lb are only used once, then I would inline them. giving them short names only adds unnecessary indirection • personally, I would prefer reading first-body to fb. even better, first-binding and last-binding

phronmophobic21:07:47

in general, I would try to structure the code to reveal the high level algorithm (which is sort of subjective). for me, the high level algorithm psuedo code is like:

(defn expression->where-clause
  [attrs in-binding find-binding]

  (let [->b #(symbol (str "?" %))
         ;; make queries using (drop (butlast attrs))
        middle-queries (stuff (drop (butlast attrs)) ...)

        first-query [find-binding (first attrs) (->b 0)]
        last-query [(->b (dec (count attrs))) (last attrs) in-binding]
        ]
    (vec
     (concat
      [first-query]
      middle-queries
      [last-query])))
  )

phronmophobic21:07:00

if you are going to have special cases based on the number of attrs, I would prefer a case to a cond:

(case (count attrs)
  0 []
  1 (into [find-binding] (conj attrs in-binding))
  2 [[find-binding f_attrs (->b (name f_attrs))]
     [(->b (name f_attrs)) l_attrs in-binding]]
  
  ;; else
  (let [] ...))

jaihindhreddy21:07:01

Assuming the intermediary symbols are free-variables, you can use gensym. partition can produce sliding windows when called with 3 args. map can take multiple collections. All these combine to make this happen:

(defn expr->where-clause [attrs from to]
  (let [syms (->> #(symbol (str "?" (gensym))))
                  (repeatedly (dec (count attrs)))]
    (->> (concat [from] syms [to])
         (partition 2 1)
         (map (fn [a [x y]] [x a y]) attrs))))
Of course, there is still a chance the generated symbols may conflict with from and/or to. You can tweak the fn to prevent that.

jaihindhreddy22:07:17

As you said, interleave works too, but it looks a bit more awkward to my eyes:

(defn expr->where-clause [attrs from to]
  (let [syms (->> #(symbol (str "?" (gensym)))
                  (repeatedly (dec (count attrs))))]
    (->> (conj (vec syms) to)
         (interleave attrs)
         (cons from)
         (partition 3 2))))

drewverlee23:07:59

@U883WCP5Z nice. That's fairly clean. I tried and failed to get parition and interleave to get this to work. i think i was relying on the padding and not cons the from.

drewverlee23:07:13

@U7RJTCH6J thanks for the review. i cleaned it up based on this then went another direction using reduce then ponged back to what jaihindhreedy has there. 2 hours of fiddling. 🙂

🙂 3
jaihindhreddy21:07:01
replied to a thread:Code puzzle and a review rolled into one. I need a way to translate essential the get-in list args into a datomic where clause. [] -&gt; [[]] [:a] -&gt; [[?r0 :a ?r1]] [:a :b] -&gt; [[?r0 :a ?r1] [?r1 :b ?r2]] fn: expression, in-binding, find-binding -&gt; where-clause where expression is like the get-in arg [:a :b ... ] Additionally the first and last bindings (r?0 and ?r2) are passed into the function. This is my first attempt that works: (defn expression-&gt;where-clause [attrs in-binding find-binding] (let [-&gt;b #(symbol (str "?" %)) f_attrs (first attrs) l_attrs (last attrs)] (cond (= 0 (count attrs)) [] (= 1 (count attrs)) (into [find-binding] (conj attrs in-binding)) (= 2 (count attrs)) [[find-binding f_attrs (-&gt;b (name f_attrs))] [(-&gt;b (name f_attrs)) l_attrs in-binding]] :else (let [b (butlast (rest attrs)) body (:coll (reduce (fn [{:keys [coll c]} n] {:coll (conj coll (-&gt;b c) n (-&gt;b (inc c))) :c (inc c)}) {:coll [] :c 0} b)) fb (first body) lb (last body)] (partition 3 (into [find-binding f_attrs fb] (conj body lb l_attrs in-binding))))))) I feel like interleave should be at play here but when i wasn't sure it fit so i started with reduce to get something more basic.

Assuming the intermediary symbols are free-variables, you can use gensym. partition can produce sliding windows when called with 3 args. map can take multiple collections. All these combine to make this happen:

(defn expr->where-clause [attrs from to]
  (let [syms (->> #(symbol (str "?" (gensym))))
                  (repeatedly (dec (count attrs)))]
    (->> (concat [from] syms [to])
         (partition 2 1)
         (map (fn [a [x y]] [x a y]) attrs))))
Of course, there is still a chance the generated symbols may conflict with from and/or to. You can tweak the fn to prevent that.