code-reviews

2018-12-28T14:15:05.000900Z

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

2018-12-28T14:15:10.001100Z

(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"))))

2018-12-28T14:15:42.001700Z

the finally needs to have the try as immediate parent

2018-12-28T14:15:58.002100Z

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

jumar 2018-12-28T17:04:10.005900Z

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.

2018-12-28T18:24:32.006300Z

I see

2018-12-28T18:24:41.006600Z

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

seancorfield 2018-12-28T18:31:34.008600Z

(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))

seancorfield 2018-12-28T18:32:34.009400Z

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

seancorfield 2018-12-28T18:33:11.009700Z

^ @mping How does that look to you?

2018-12-28T18:40:59.010300Z

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

2018-12-28T18:41:03.010600Z

but I see your point

seancorfield 2018-12-28T18:58:52.010900Z

That does log the status and time together.

seancorfield 2018-12-28T18:59:45.011600Z

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