Defining a map object inside a loop recur - Clojure

Question

I am trying to use a loop recur that creates an empty map inside the loop. For each entry in the loop (loops through a vector of maps) it will see if there is a key in the newly created map that matches the iterated values key and if not create one.

I have created this code:

(def meteor-map (json/read-str (clojure.string/lower-case
(slurp "https://data.nasa.gov/resource/y77d-th95.json"))))

(defn most-falls [values]
(loop [values map count-tracker{}]
  (if (empty? values)
    (count-tracker)
    (do
      (def key (keyword (get (first values) "year")))
      (if (contains? (first values) key)
        (do
          (def count-tracker (update count-tracker key inc))
          (recur (rest values) count-tracker)
        )
        (do
          (def count-tracker (assoc count-tracker key 1))
          (recur (rest values) count-tracker)
        )
      )
    )
  )
)
)

(most-falls meteor-map)

However when I call this function and pass in meteor-map (which is a vector of maps) i get an error saying

wrong number of args (0) passed to persistentarraymap

I think this could be due to how I am creating the initial count-tracker object inside the loop creation but I am unsure.

Any ideas?

Thanks

PS am aware this question is a bit vague so any questions just ask!

What's the exact error? The problem is, your code as multiple issues (using def, and [values map in loop), so it's hard to pin down what exactly is going wrong. — Carcigenicate
– Carcigenicate, Commented Jan 2, 2018 at 23:38
What is (count-tracker) supposed to be doing? That's your error. You're trying to call the map without arguments. Once you fix that though, values map will give you an error since map is a function. I think you just meant values values. — Carcigenicate
– Carcigenicate, Commented Jan 2, 2018 at 23:43
@Carcigenicate count-tracker is supposed to be an empty map I create and then add/update keys to depending on the value that is currently in the loop. For example if the first value has a :year of "1996" and that doesnt already exist in count-tracker it will add that key to that map. If it existed already the value (eg :1996 1) would be incremented. hope that makes sense! — broliverparker
– broliverparker, Commented Jan 2, 2018 at 23:49
But what are you trying to do with (count-tracker)? Just return count-tracker? Then remove the parenthesis around it. Remember, if you surround something in parentheses, you're calling it as a function. — Carcigenicate
– Carcigenicate, Commented Jan 2, 2018 at 23:51

Carcigenicate · Accepted Answer · 2018-01-04 19:33:48Z

There's multiple things here to be brought up. Your main problem though is with (count-tracker). You're surrounding the map in parenthesis, which means that you want to call it as a function. You can't arbitrarily add parentheses to code like you can in other languages; it has a very specific meaning in Clojure. (f) always means that the function f is being called. Just change it to count-tracker to return the value.

Other things:

NEVER use def inside of a function unless it's necessary. In this case though, it's entirely unnecessary. Every use of def creates globals that last for the length of the program (yes, they exist even after the function exits!). Use let instead:
```
(let [key (keyword (get (first values) "year")))]
   ... ) ; Use key here
```
(loop [values map ...] will cause errors as well. map is a function, so this throws away the argument passed in to most-falls, overwriting it with the map function. This will cause an error when you try to use values as a sequence, since the map function doesn't support empty? or first, or anything else you're trying to use it for. I think you just intended to just rebind the argument to be used in the loop. Just change it to (loop [values values ...]. Arguably, you shouldn't shadow arguments by creating other bindings with the same name, but that's not exceedingly important here.

There's still a couple other petty things that could be improved. By using destructuring you could skip the calls to first and rest, and using reduce could simplify the explicit looping using loop, but those would detract from the main issues. Taking into consideration what I mentioned above, I'd write your function as:

(defn most-falls [values]
  (loop [values values
         count-tracker {}]
    (if (empty? values)
      count-tracker
      (let [key (keyword (get (first values) "year"))]
        (recur (rest values)
               (if (contains? (first values) key)
                 (update count-tracker key inc)
                 (assoc count-tracker key 1)))))))

Ivan Grishaev · Accepted Answer · 2018-01-04 12:41:41Z

First, there is definitely an issue in that clause:

(loop [values map count-tracker{}]

I'm not use what did you try to achieve, but please take a look.

Next, never use def or defn forms inside the code, only on the top level of a namespace.

Finally, the loop/recur is pretty low-level form and should be used with a strong knowledge of what you are doing. More often, it might be replaced with more user-friendly ones. The reduce would be a good one I believe. It takes an initial value (an empty map in your case), a collection and a function of two arguments where the second one is the current collection's item and the second one either an initial value or a result of the previous function call.

Inside that function, you decide whether your map has some specific key and add it if it does not.

Short example:

# here are some data you've read from a file
(def items [{...} {...} {...}])

# reduce process function
(defn process
  [result item]
  (if (:some-key result) ;; here, you check the current map for a key
    result               ;; return the old map if everything is ok
    (assoc result :some-key some-data)))  ;; accumulate a new key into a map

(reduce process {} items)

Collectives™ on Stack Overflow

Defining a map object inside a loop recur - Clojure

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related