A crude clojure progress reporting function

Question

The purpose of this function is to report processing progress to the terminal. It loops through an array of maps that contain two properties: :sequence and :function. Within each sequence-function-map, it will loop through the sequence, and run the function on each item in the sequence.

This is my first Clojure program that will be used in a production environment. This particular function does the job, but I suspect that it's not very lispy.

(defn run-with-reporting [sequence-function-maps stop-time]
  (def beginning-time (java.util.Date.))
  (let [seq-counts (map #(count (:sequence %)) sequence-function-maps)]
    (def total-sequence-counts (reduce + seq-counts)))
  (println (str "total counts = " total-sequence-counts))
  (def prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0}))
  (doseq [sequence-function-map sequence-function-maps]
    (let [sequence (:sequence sequence-function-map),
          function (:function sequence-function-map)]
      (loop [loop-seq sequence]

        (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
          (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
          (if loop-seq
            (do
              (def item (first loop-seq))
              (def start-time (java.util.Date.))
              (function item)
              (def end-time (java.util.Date.))
              (def time-delta (- (.getTime end-time) (.getTime start-time)))
              (let [derefd-rpt (deref prog-rpt-ref)]
                (dosync (alter prog-rpt-ref assoc
                               :done (inc (:done derefd-rpt)),
                               :total-time-taken (+ (:total-time-taken derefd-rpt) time-delta))))
              (let [derefd-rpt (deref prog-rpt-ref)]
                (let [average-time (/ (:total-time-taken derefd-rpt) (:done derefd-rpt))]
                  (println "Avg time / each = " (hrs-min-sec average-time)
                           ", Estimated total time left = "
                           (hrs-min-sec (* average-time (- (:todo derefd-rpt) (:done derefd-rpt)))))))
              (recur (next loop-seq))))))))
  (let [derefd-rpt (deref prog-rpt-ref)]
    (println "Total time taken = " (hrs-min-sec (- (.getTime (java.util.Date.)) (.getTime beginning-time))) ", Done = " (:done derefd-rpt) "/" (:todo derefd-rpt))))

Here's a testing function

(defn test-run-w-reporting [stop-clj-time]
  (def testing-func (fn [item] (. java.lang.Thread sleep (rand 1000))))
  (let [sequence-function-maps [{:sequence (range 30), :function testing-func},
                               {:sequence (range 15), :function testing-func}]]
    (run-with-reporting sequence-function-maps stop-clj-time)))

Answer 1

I will point out stuff that I'd do differently, though I am by no means a clojure expert, I do have some experience in it. Numbered items will reappear inside the code as comments.

1. no DEF inside defn body
   • def will define the var for your entire namespace as such, you will pollude your namespace with temporaries
   • use LET instead
2. no DO necessary inside the body of IF
   • furthermore, I find it more idiomatic to use WHEN in the case that no else branch exists
3. using @ instead of DEREF could reduce verbosity
4. by using UPDATE-IN instead of INC, you can simply pass a transformation function
   • note the vector notation however: UPDATE-IN takes a vector of keywords
5. You can pass a partial function, i.e. a function which is partially applied as the transformation function to update the total-time-taken
6. No need to nest LETs, you can write all definitions into a single let

Code:

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (java.util.Date.) ;; 1.
        seq-counts (map #(count (:sequence %)) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0})]
    (println (str "total counts = " total-sequence-counts))
    (doseq [sequence-function-map sequence-function-maps]
      (let [sequence (:sequence sequence-function-map),
            function (:function sequence-function-map)]
        (loop [loop-seq sequence]
          (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
            (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
            (when loop-seq ;; 2.
              (let [item (first loop-seq)
                    start-time (java.util.Date.)]
                (function item)
                (let [end-time (java.util.Date.)
                      time-delta (- (.getTime end-time) (.getTime start-time))
                      derefd-rpt @prog-rpt-ref]         ;; 3.
                  (dosync (alter prog-rpt-ref update-in ;; 4.
                                 [:done] inc,
                                 [:total-time-taken] (partial + time-delta))) ;; 5.
                  (let [derefd-rpt @prog-rpt-ref ;; 3. & 6.
                        average-time (/ (:total-time-taken derefd-rpt) (:done derefd-rpt))]
                    (println "Avg time / each = " (hrs-min-sec average-time)
                             ", Estimated total time left = "
                             (hrs-min-sec (* average-time (- (:todo derefd-rpt) (:done derefd-rpt)))))))
                (recur (next loop-seq))))))))
    (let [derefd-rpt (deref prog-rpt-ref)]
      (println "Total time taken = " (hrs-min-sec (- (.getTime (java.util.Date.)) (.getTime beginning-time))) ", Done = " (:done derefd-rpt) "/" (:todo derefd-rpt))))
  )

1. I would try to reduce verbosity and increase readability (in the literate sense) by utilising descriptive helper functions - current-time instead of (.getTime (java.util.Date.))

2. Personally I like to keep the amount of variables as low as possible. Variables are great for producing side-effects, describing complex results or for when you need a result several times. In the case of the ref, writing @ instead of using a temporary seems more natural to me.

   As such:

   • I would try to reduce the amount of side-effects and
   • use descriptive functions to calculate complex results and place them where they belong without temporary variables
3. You can use destructring for maps
   • (let [{:keys [key1 key2]} mymap] ...) will let-bind key1 and key2 by extracting :key1 & :key2 from mymap

Code:

(defn current-time
  "Return the current time. Same as (.getTime (java.util.Date.))."
  []
  (.getTime (java.util.Date.)))

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (current-time) ;; 1.
        seq-counts (map #(count (:sequence %)) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        prog-rpt-ref (ref {:todo total-sequence-counts, :done 0, :total-time-taken 0})]
    (println (str "total counts = " total-sequence-counts))
    (doseq [sequence-function-map sequence-function-maps]
      (let [{:keys [sequence function]} sequence-function-map] ;; 9.
        (loop [loop-seq sequence]
          (if (and stop-time (clj-time/after? (clj-time/now) stop-time))
            (println "stopped at " (clj-time/now) ". Requested stop at " stop-time)
            (when loop-seq ;; 2.
              (let [item (first loop-seq)
                    start-time (current-time)] ;; 7.
                (function item)
                (dosync (alter prog-rpt-ref update-in ;; 4.
                               [:done] inc,
                               [:total-time-taken] (partial + (- (current-time) start-time)))) ;; 5., 7. & 8.
                (let [average-time (/ (:total-time-taken @prog-rpt-ref) (:done @prog-rpt-ref))] ;; 8.
                  (println "Avg time / each = " (hrs-min-sec average-time)
                           ", Estimated total time left = "
                           (hrs-min-sec (* average-time (- (:todo @prog-rpt-ref) (:done @prog-rpt-ref)))))) ;; 8.
                (recur (next loop-seq))))))))
    (println "Total time taken = " (hrs-min-sec (- (current-time) beginning-time))
             ", Done = " (:done @prog-rpt-ref)
             "/" (:todo @prog-rpt-ref))))

Now, lets take a look at the function itself. In general, when writing in clojure you should try to keep your functions generic and simple. By keeping them simple and focusing on only a single task, they are usually easier to compose and reuse. I would thus recommend splitting up your function.

In your case what you want to do is really:

1. map a function over a sequence: clojure already provides a function for that, namely MAP.
2. stop the mapping operation if a specified time has been reached.
3. print out the average time per call
4. print out total time taken

The first point is trivial.

For the second point, we can exploit the fact that clojure has lazy sequences. This allows us to use the built-in TAKE-WHILE function with a predicate which checks the time and stops when it has passed your stop-time:

(defn take-until
  "Returns a lazy seq of items from coll util the STOP-TIME has been reached."
  [stop-time coll]
  (take-while
   (fn [item]
     (if (not (> (current-time) stop-time))
       true
       (do (println "stopped at " (current-time) ". Requested stop at " stop-time)
           false)))
   coll))

The third point can be achieved by simply mapping over a collection and printing out the necessary information. This again is really just what you did, but refactored into a function that works on any sequence:

(defn measure-coll-retrieval
  "Returns a lazy seq of items from coll. Will print to stdout the
  average time between element extractions."
  ;; you can use destructuring inside the argument list as well, here
  ;; we're additionaly specifying some defaults for when the caller
  ;; does not provide a value
  [coll & {:keys [start-count total-count] :or {start-count 0 total-count nil}}]
  (let [beginning-time (current-time)]
    (map-indexed
     (fn [index item]
       (let [index (+ start-count index 1)
             average-time (/ (- (current-time) beginning-time) index)]
         (print "Avg time / each = " (hrs-min-sec average-time))
         (if total-count
           (println ", Estimated total time left = "
                    (hrs-min-sec (* average-time (- total-count index))))
           (println))
         item))
     coll)))

Before writing the reporting function, a little helper so we can get around seq-chunking in clojure:

(defn unchunk
  "takes a chunked sequence and turns it into an unchunked sequence"
  [s]
  (lazy-seq
   (when-let [[x] (seq s)]
     (cons x (unchunk (rest s))))))

Now we can write the actual reporting function:

(defn run-with-reporting [sequence-function-maps stop-time]
  (let [beginning-time (current-time) ;; 1.
        seq-counts (map (comp count :sequence) sequence-function-maps)
        total-sequence-counts (reduce + seq-counts)
        intermed-count (atom 0)]
    (println (str "total counts = " total-sequence-counts))
    ;; note how it is possible to immediately destructure the map
    (doseq [{:keys [sequence function]} sequence-function-maps]
      (swap! intermed-count
             +
             (count
              ;; this is where everything happens: apply function,
              ;; take-until stop-time reached & measure average time
              (measure-coll-retrieval
               (take-until stop-time (map function (unchunk sequence)))
               :start-count @intermed-count
               :total-count total-sequence-counts))))
    (println "Total time taken = " (hrs-min-sec (- (current-time) beginning-time))
             ", Done = " @intermed-count
             "/" total-sequence-counts)))

(defn test-run-w-reporting [stop-clj-time]
  (def testing-func (fn [item] (. java.lang.Thread sleep (rand 1000))))
  (let [sequence-function-maps [{:sequence (range 30), :function testing-func},
                                {:sequence (range 15), :function testing-func}]]
    (run-with-reporting sequence-function-maps stop-clj-time)))

I hope this has helped you some.

Kind regards, Thomas