I am working out a bit of Clojure code that will take a ref to a map and increment a key value pair in the map. I think I am using ref correctly, but Im not sure about atom. Do I need to use swap! to be more idiomatic? I am new to STM and Clojure, does this look thread-safe / sane? What am I missing?
(defn increment-key [ref key]
(dosync
(if (= (get @ref key) nil)
(alter ref assoc key (atom 1))
(alter ref assoc key (atom (inc @(get @ref key)))))))
(defn -main [& args]
(def my-map (ref {}))
(increment-key my-map "yellow")
(println my-map)
(increment-key my-map "yellow")
(println my-map))
Prints
$ lein run
#<Ref@494eaec9: {yellow #<Atom@191410e5: 1>}>
#<Ref@494eaec9: {yellow #<Atom@7461373f: 2>}>
It's not very idiomatic Clojure to do this: embedding mutable objects within a persistent data structure generally defeats the whole point of immutable data structures.
I would avoid the internal atom entirely, and just have a number associated with the key. Then the entire data structure contained within my-map
will be immutable.
As for the thread safety: it really depends on how you are going to use it. A ref
appears to be overkill in this case as it is only really needed when you need to co-ordinate transactions across multiple refs, which you don't have here. Probably an atom
is sufficient for what you are trying to do.
Here's how you might tackle it instead:
(defn increment-key [ref key]
(swap! ref update-in [key] (fn [n] (if n (inc n) 1))))
(def my-map (atom {}))
(increment-key my-map "yellow")
(println my-map) ;; => {"yellow" 1}
(increment-key my-map "yellow")
(println my-map) ;; => {"yellow" 2}
EDIT: even better would be to keep mutability out of your functions and define increment-key as a pure function.
(defn increment-key [m key]
(assoc m key (if-let [n (m key)] (inc n) 1)))
(def my-map (atom {}))
(swap! my-map increment-key "yellow")
(println my-map) ;; => {"yellow" 1}
(swap! my-map increment-key "yellow")
(println my-map) ;; => {"yellow" 2}