code-reviews 2022-03-17

@francesco.losciale

(defn remove-vehicle-original
  [truck vehicles]
  (let [index (.indexOf vehicles truck)
        length (.length vehicles)]
    (into [] (concat (subvec vehicles 0 index) (subvec vehicles (inc index) length)))))

(defn remove-vehicle-idiomatic
  [truck vehicles]
  (into []
        (remove #{truck})
        vehicles))
(ins)user=> (remove-vehicle-original "bar" ["foo" "bar" "baz"])
["foo" "baz"]
(ins)user=> (remove-vehicle-idiomatic "bar" ["foo" "bar" "baz"])
["foo" "baz"]
(cmd)user=>

also, I think you can just use (remove #{truck} vehicles) on its own, since you only need vectors because you were relying on indexing - not sure though

πŸ‘ 1

correct I could just use remove without copying to vectors, it works

on a more basic level - a function that complex should either be reduced to something simpler or lifted out and put in a defn like I did to test my replacement above

πŸ‘ 1

@francesco.losciale I also see that the same anonymous function is copy pasted into multiple parts of the code, I hope I don't have to explain why that's a bad idea

perhaps what you are missing here is that you can pass any number of args to update-in? you don't need to capture the other args lexically in a closure

(ins)user=> (update-in {:a {:b 21}} [:a :b] * 2)
{:a {:b 42}}