Fork me on GitHub
#code-reviews
<
2020-02-16
>
hindol18:02:50

Hi, I have the following code,

(defn error->status
  "Converts a JSON-RPC error code into the appropriate HTTP status."
  [code]
  (cond
    (= -32600 code)                400
    (= -32601 code)                404
    (#{-32602 -32603 -32700} code) 500
    (<= -32099 code -32000)        500))

(defn infer-status
  "Infer the HTTP status code from the JSON-RPC response.
   See: "
  [body]
  (if-let [error (:error body)]
    (if-let [code (:code error)]
      (if (int? code)
        (if-let [status (error->status code)]
          status
          (throw (ex-info "Error code invalid!"
                          {:response body})))
        (throw (ex-info "Error code is not a number!"
                        {:response body})))
      (throw (ex-info "Error code missing from error response!"
                      {:response body})))
    200))
What would be a more sane way to write the infer-status part? The nested ifs are getting out of hand.

seancorfield18:02:25

@hindol.adhya I guess I'd ask if you really need the different exceptions? Are you surfacing that message somewhere?

hindol18:02:21

Yes, it is getting logged elsewhere. The different messages are good to have.

seancorfield18:02:56

Given that you're "only" throwing one exception type and ex-data is going to be identical in each case, I'd probably collapse those down into one error -- you can figure out the specifics based on ex-data after the fact.

hindol18:02:12

If it's not too much, some sample code for this approach will be really helpful.

hindol18:02:24

Like how this approach is being used in real projects.

seancorfield18:02:41

See the code I pasted it the main channel.

👍 4
seancorfield18:02:41

I'd also probably use Spec to validate the response, and just log the explain-str.

hindol18:02:07

The thought of using spec did occur to me. I have never used it before, so just procrastinating I guess.

seancorfield18:02:51

(if (s/valid? ::response-code body)
  (if-let [status (some-> body :error :code error->status)]
    status
    (throw (ex-info "Invalid/missing error code" {:response body :failure (s/explain-str ::response-code body)})
  200)

seancorfield18:02:33

The ::response-code spec would have error as an optional key, and it would be specified to have a require code field that is int?.

seancorfield18:02:47

(s/def ::code int?)
(s/def ::error (s/keys :req-un [::code]))
(s/def ::response-code (s/keys :opt-un [::error]))
off the top of my head.

hindol18:02:53

Thank you so much for this! I have to try this out. Looks much cleaner to me.

hindol18:02:03

A very tangential question, why do we :req-un and yet give a fully qualified keyword in the vector?

seancorfield18:02:18

Your structure has unqualified keywords but specs are always qualified keywords.

seancorfield19:02:02

The namespace part of the keyword is to make the spec unique (in your case so they don't collide with any other :error and :code mappings in your codebase).

hindol19:02:09

in your case so they don't collide with any other :error and :code mappings in your codebase - don't think I understand but that might just be because I am very inexperienced with spec.

seancorfield19:02:48

I just updated the spec above -- I wasn't writing what I intended!

seancorfield19:02:56

Another option would be:

(s/def :error/code int?)
(s/def  :response/error (s/keys :req-un [:error/code]))
(s/def ::response-code (s/keys :opt-un [:response/error]))
Is that clearer?

seancorfield19:02:54

You could have :error/code and :success/code and other :code semantics in different places in your code.

seancorfield19:02:25

Spec uses qualified keywords, to describe both qualified keywords and unqualified keywords 🙂

👍 4
hindol19:02:09

This does look clearer. Thanks!