Fork me on GitHub
#code-reviews
<
2018-12-28
>
mping14:12:05

Hi all, how would you fix the following mutable-like code?

mping14:12:10

(let [start    (System/currentTimeMillis)
           response (atom nil)]
       (try
         (log/info "request:" url)
         (reset! response (client/get url opts))
         (:body @response)
         (finally
           (log/info "status" (:status @response) "duration:" (- (System/currentTimeMillis) start) "ms"))))

mping14:12:42

the finally needs to have the try as immediate parent

mping14:12:58

so I can’t use let and nest the finally within the let

jumar17:12:10

I'd probably use catch clause and log "error" there and moved "info" log to the "happy path" flow. The log would be somewhat duplicated but I guess you can create a little function to cover that.

mping18:12:41

I was wondering if I could not use an atom, but I can’t really see how

seancorfield18:12:34

(let [start (System/currentTimeMillis)
      _ (log/info "request:" url)
      response (try (client/get url opts) (catch Exception e …))]
  (log/info "status" (:status response) "duration:" (- (System/currentTimeMillis) start) "ms")
  (:body response))

seancorfield18:12:34

In the ... of the catch you can log errors and/or return a response map with appropriate :status and/or :body fields.

seancorfield18:12:11

^ @mping How does that look to you?

mping18:12:59

looks good; I would prefer to log the status and time at the same time so I dont have to use some correlation on the logs

mping18:12:03

but I see your point

seancorfield18:12:52

That does log the status and time together.

seancorfield18:12:45

(or are you concerned that the ... error handling will take enough that that it would affect the millisecond timings?)