• 1

• 1.8k
• 10
• 23

• 1.8k
• 10
• 23

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 the world to test:

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