Fork me on GitHub
#code-reviews
<
2022-03-17
>
Fra15:03:35

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

noisesmith19:03:05

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

noisesmith19:03:13

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
Fra09:03:33

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

noisesmith19:03:05

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
noisesmith19:03:56

@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

noisesmith19:03:59

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