Hi all, I have a code review request, if anyone has a min. I’m writing some code to interface with APIs, requesting creation of reports, waiting for creation, downloading or terminating.

My problem is that it looks very imperative, not very clojure-y:

(defn download-stats
  "Requests creation of `report-type` for `date`. Once the report job is
   BUILT, returns the data as a java.io.BufferedReader. The report is in tsv
   format.

   Cleans up after itself by deleting requested job."
  [customer-id report-type date]
  (let [c  (creds customer-id)
        id (-> (reports.stat/create! c {:reportTp report-type :statDt date})
               :body :reportJobId)]
    (loop [job (reports.stat/fetch-job c id)]
      (Thread/sleep 50)
      (if-not (#{"RUNNING" "REGIST"} (:status job))
        (if (= "BUILT" (:status job))
          (let [buf-r (reports.common/download c job)]
            (reports.stat/delete! c id)
            buf-r)
          (do
            (reports.stat/delete! c id)
            (throw
             (Exception.
              (str "Requested job returned status of: " (:status job))))))
        (recur (reports.stat/fetch-job c id))))))

Any pointers would be much appreciated!

@sooheon To avoid the duplicated call to delete! I'd probably wrap the loop in try / finally (containing the delete! call) to make it clearer this is a post-operation cleanup. That would allow the inner let to be removed. I'd use a case to avoid the nested if.

(defn download-stats
  "Requests creation of `report-type` for `date`. Once the report job is
   BUILT, returns the data as a java.io.BufferedReader. The report is in tsv
   format.

   Cleans up after itself by deleting requested job."
  [customer-id report-type date]
  (let [c  (creds customer-id)
        id (-> (reports.stat/create! c {:reportTp report-type :statDt date})
               :body :reportJobId)
        fetch (fn []
                (Thread/sleep 50)
                (reports.stat/fetch-job c id))]
    (try
      (loop [job (fetch)]
        (case (:status job)
          ("RUNNING" "REGIST")
          (recur (fetch))

          "BUILT"
          (reports.common/download c job)

          (throw
           (Exception.
            (str "Requested job returned status of: " (:status job))))))
      (finally
        (reports.stat/delete! c id)))))

Also, since you're fetching the job then sleeping, I'd probably add a helper function that accepted c and id and slept first, then fetched the job -- I'll edit the above code.

Great, try/catch/throw/finally are constructs I need to get more familiar with, this was a really helpful example. Thanks @seancorfield