Adding/removing map values: unsymmetric implementation

Question

I am learning Clojure by implementing a simplistic resource reservation system. At its core are the two functions for placing/canceling reservations for a given resource:

; thread-safe map of {resource -> #{reservations}}
(def known-reservations (atom {}))

(defn place-reservation
  [new-res]
  (swap! known-reservations
         #(merge-with clojure.set/union % {(:resource new-res) #{new-res}})))

(defn cancel-reservation
  [canceled-res]
  (swap! known-reservations
         #(update % (:resource canceled-res) disj canceled-res)))

They seem to work correctly, but the lack of symmetry bothers me. Placing and canceling reservations are symmetric operations, but this is not reflected by the above code: merge-with clojure.set/union doesn't even resemble update ,,, disj.

I tried the following variant:

(defn place-reservation
  [new-res]
  (swap! known-reservations
         #(update % (:resource new-res) conj new-res)))

but since conj nil :x returns a list rather than a set, this leads to duplicate reservations which I want to prevent.

What is the most idiomatic fix to my problem?

PS: if anyone can come up with a more expressive question title, I'll thankfully accept the edit!

edit: here's a simple usage example for experimentation:

(defrecord Resource [name])
(defrecord Reservation [^Resource resource user])

(def car (->Resource "car"))
(def my-res (->Reservation car "me"))
(def your-res (->Reservation car "you"))

(place-reservation my-res)
(place-reservation your-res)
(place-reservation my-res) ;; duplicate reservation must not change anything

(cancel-reservation my-res)

Answer 1

I agree that the lack of symmetry in your implementation of place-reservation using merge-with and union is bothersome. Fortunately, there's a function to fix that!

fnil

The fnil function allows you to create a function that acts like an existing function, but replaces the first argument with something else in the case that it's given nil:

(conj nil :x)
;=> (:x)

((fnil conj #{}) nil :x)
;=> #{:x}

So you can write place-reservation like this:

(defn place-reservation [new-res]
  (swap! known-reservations
         #(update % (:resource new-res) (fnil conj #{}) new-res)))

swap!

As you know, swap! will pass the current state of the atom as the first argument to the function you provide, but you can give values for the other arguments as well. So instead of passing a function that calls update with the current state and some other arguments, you can just pass update and those other arguments directly, and swap! will do the rest:

(defn place-reservation [new-res]
  (swap! known-reservations update (:resource new-res) (fnil conj #{}) new-res))

(defn cancel-reservation [canceled-res]
  (swap! known-reservations update (:resource canceled-res) disj canceled-res))

Global state

Currently, you store known-reservations in a global var. This is a Bad Thing. Even if you don't plan to have multiple atoms of reservation lists in your actual program, it makes your code more painful to test:

(require '[clojure.test :as t])

(t/deftest reservation-test
  (reset! known-reservations {})
  (place-reservation my-res)
  (place-reservation your-res)
  (place-reservation my-res)
  (t/is (= {car #{my-res your-res}} @known-reservations))
  (reset! known-reservations {}))

I suppose you could make this a bit more concise using fixtures, but either way, if you eventually end up with enough tests that you'd like to run them in parallel, you'll be out of luck.

The easiest way to fix this problem is to just take the state as a parameter instead:

(defn place-reservation [known-reservations new-res]
  (swap! known-reservations update (:resource new-res) (fnil conj #{}) new-res))

(defn cancel-reservation [known-reservations canceled-res]
  (swap! known-reservations update (:resource canceled-res) disj canceled-res))

This still isn't great, but it's an improvement:

(t/deftest reservation-test
  (t/is (= {car #{my-res your-res}}
           @(doto (atom {})
              (place-reservation my-res)
              (place-reservation your-res)
              (place-reservation my-res)))))

Pure functions

In place-reservation and cancel-reservation, you complect two things:

• The logic of placing or canceling reservations
• The fact that you're storing reservations in an atom

You'd be better off writing place-reservation and cancel-reservation as pure functions that take a reservation collection and return a new reservation collection:

(defn place-reservation [known-reservations new-res]
  (update known-reservations (:resource new-res) (fnil conj #{}) new-res))

(defn cancel-reservation [known-reservations canceled-res]
  (update known-reservations (:resource canceled-res) disj canceled-res))

Pure functions are the easiest things in the world to test:

(t/deftest reservation-test
  (t/is (= {car #{my-res your-res}}
           (reduce place-reservation {} [my-res your-res my-res]))))