code-reviews

Fra 2022-03-17T15:40:35.272869Z

Hi, I don’t know how many are familiar with https://github.com/Softwarepark/exercises/blob/master/transport-tycoon-1.md , I was wondering if anyone could review my solution https://gist.github.com/francesco-losciale/067d31aaef6443107c222babf9bf2675? Thanks

2022-03-17T19:22:37.797629Z

@francesco.losciale correct me if I'm wrong, but this looks a lot like an overly complicated remove https://gist.github.com/francesco-losciale/067d31aaef6443107c222babf9bf2675#file-tt-kata_core-clj-L29

2022-03-17T19:24:05.662279Z

@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=>

2022-03-17T19:25:13.798409Z

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
Fra 2022-03-18T09:25:33.617419Z

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

2022-03-17T19:26:05.939099Z

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
2022-03-17T19:28:56.456129Z

@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

2022-03-17T19:30:59.138079Z

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}}