Clojure: Idiomatic use of atom and ref?

Question

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

Answer 1

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}