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.
- 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
- 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
- using @ instead of DEREF could reduce verbosity
- 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
- You can pass a partial function, i.e. a function which is
partially applied as the transformation function to update the total-time-taken
- 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))))
)
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.))
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
- 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:
- map a function over a sequence: clojure already provides a function for that, namely MAP.
- stop the mapping operation if a specified time has been reached.
- print out the average time per call
- 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