Encapsulated state in clojure

While going through SICP and trying to implement the code in clojure, I've found that while I can get the code in chapter 3 to work, it seems to go against Clojure idioms, but I can't quite imagine the "Clojure way" to do it either.

Take for example, the digital circuit simulator. This is my implementation:

(declare call-each)
(defn make-wire []
  (let [signal-value (atom 0)
        action-procedures (atom '())
        set-my-signal (fn [new-value]
                        (if-not (= @signal-value new-value)
                          (do (reset! signal-value new-value)
                              (call-each @action-procedures))
                          'done))
        accept-action-procedure! (fn [proc]
                                   (swap! action-procedures
                                          conj proc)
                                   (proc))
        dispatch (fn [m]
                   (cond (= m 'get-signal) @signal-value
                         (= m 'set-signal) set-my-signal
                         (= m 'add-action) accept-action-procedure!
                         :else (error "Unknown operation -- WIRE" m)))]
    dispatch))

(defn call-each [[proc & rest :as procedures]]
  (if (empty? procedures)
    'done
    (do (proc)
        (call-each rest))))

(defn get-signal [wire]
  (wire 'get-signal))
(defn set-signal! [wire new-value]
  ((wire 'set-signal) new-value))
(defn add-action! [wire action-procedure]
  ((wire 'add-action) action-procedure))

As you can see, it has an internal signal-value and action-procedures as state. Is this the natural way to do this in Clojure, and if not, how would you do it?

There are a few problems.

-
You should not read the value from an atom and update it conditional on its value, as in set-my-signal. It's value may have changed in between those two calls. In this case refs instead of atoms may need to be used.

-
Do not use unbounded recursion as in call-each.

Instead of:

(defn call-each [[proc & rest :as procedures]]
  (if (empty? procedures)
    'done
    (do (proc)
        (call-each rest))))

You can do:

(defn- call-each [procedures]
  (doseq [proc procedures] (proc)))

-
error is not a clojure function. So you should have implemented in clojure with throw... Just use throw instead. It would read something like: (throw (UnsupportedOperationException. (str "Unknown operation -- WIRE " m) nil)). Or instead of throwing an exception you can return nil. So it would be easier to extend (add new messages by decorating) the make-wire constructor.

-
Case analysis (cond) on a constant symbols is someone does a lot in Scheme. Clojure maps are more readable though. Also using keywords :get-signal instead of symbols 'get-signal is more conventional in accessing the fields of a map, record, so on..

make-wire would be read something like this, if I wrote it in Clojure:

(defn make-wire []
  (let [signal-value (ref 0)
        action-procedures (ref '())]
    {
     :get-signal (fn [] @signal-value)
     :set-signal (fn [new-value]
                   (dosync
                     (when-not (= @signal-value new-value) 
                       (ref-set signal-value new-value)
                       (call-each @action-procedures))))
     :add-action (fn [proc]
                   (dosync
                     (alter action-procedures conj proc)
                     (proc)))
     }))

The point of encapsulation as taught in SICP is that you can combine components independent of their implementation details.

So you might want to define a protocol explicitly for a wire:

(defprotocol Wire
  (get-signal [w])
  (set-signal! [w new-value])
  (add-action! [w action-procedure]))

Then whether you implement it using a record:

(defrecord WireRec [signal-value action-procedures]
  Wire
  (get-signal [_] @signal-value)
  (set-signal! [_ new-value]
    (dosync
      (when-not (= @signal-value new-value)
        (ref-set signal-value new-value)
        (call-each @action-procedures))))
   (add-action! [_ proc]
     (dosync
       (alter action-procedures conj proc)
       (proc))))

(defn make-wire2 []
  (WireRec. (ref 0) (ref '())))

Or a map:

(defn make-wire3 []
  (let [m (ref {
                :signal-value 0
                :action-procedures '()
                })]
  (reify Wire
    (get-signal [_] (:signal-value @m))
    (set-signal! [_ new-value]
          (dosync
            (when-not (= (:signal-value @m) new-value)
              (alter m assoc :signal-value new-value)
              (call-each (:action-procedures @m)))))
    (add-action! [_ proc]
      (dosync
        (alter m update-in [:action-procedures] conj proc)
        (proc))))))

should not matter.

StackExchange Code Review Q#72171, answer score: 4