2
\$\begingroup\$

I'm new to Clojure. How can I make this more idiomatic Clojure code? Among other things, there are three (do) blocks and I don't think using (def) as many times as I do is a recommended idea.

(defn add-phone
  "Add a phone number to a given service. There are a few steps we have to take:
    - Validate the service name and token
    - Validate the phone number 
    - Send a text message to the number
    - Store the number in the database
  Arguments:
    request - an authorized HTTP request
  Returns:
    201 Resource Created - phone number added to a service
    400 Bad Request - invalid phone number or bad request"
  [request auth-type]
  (do
    (println "Url requested is " (:request-method request) " " (:uri request))
    (println request)
    (def auth-header ((:headers request) "authorization"))
    (def auth-map (utils/get-auth-header auth-type auth-header))
    (if (model/invalid-user-and-pass auth-map auth-type)
        {:status 401,
         :headers {"WWW-Authenticate" "Basic realm=\"smsauth.herokuapp.com\""}}
      (do
        (def raw-phone-no (:phone_number (:params request)))
        (def service-name (:service-name auth-map))
        (if raw-phone-no
          (do 
            (def valid-phone-no (utils/parse-phone-no raw-phone-no))
            ; XXX get the service id some other way.
            (def service-map (model/service-in-db service-name))
            (def service-id (service-map :id))
            ; XXX check that service doesn't already have phone number
            (model/store-phone-no :numbers service-id valid-phone-no)
            (twilio/send-sms-verify twilio/sms-url valid-phone-no service-name)
            ; XXX jsonify this, send more information, document it.
            {:status 201,
             :body (json/json-str {
                      :service_name service-name,
                      :phone_number valid-phone-no,
                      :status "active",
                      :uri (:uri request)
                      ; should have some kind of unique id for the phone number
                      ; here.
                      ; also return the date
                    })})
          {:status 400,
            :body (json/json-str {:error "Please include a phone_number."})})))))
\$\endgroup\$
4
\$\begingroup\$

There are several errors/bad styles here:

  • First do isn't necessary, as body of function can contain several expressions
  • instead of def inside function it's better to use let that allows to "define" several variables in one step, and they will visible only inside this let block. So 2nd & 3rd do could be replaced with corresponding let
  • if variable is used only once, then you can include function call directly into map

So your code will look like:

(defn add-phone
  [request auth-type]
  (println "Url requested is " (:request-method request) " " (:uri request))
  (println request)
  (let [auth-header ((:headers request) "authorization")
        auth-map (utils/get-auth-header auth-type auth-header)]
    (if (model/invalid-user-and-pass auth-map auth-type)
      {:status 401,
       :headers {"WWW-Authenticate" "Basic realm=\"smsauth.herokuapp.com\""}}
      (let [raw-phone-no (:phone_number (:params request))
            service-name (:service-name auth-map)]
        (if raw-phone-no
          (let [valid-phone-no (utils/parse-phone-no raw-phone-no) ; XXX get the service id some other way.
                service-map (model/service-in-db service-name)
                service-id (service-map :id) ; XXX check that service doesn't already have phone number
                ]
            (model/store-phone-no :numbers service-id valid-phone-no)
            (twilio/send-sms-verify twilio/sms-url valid-phone-no service-name)
                                        ; XXX jsonify this, send more information, document it.
            {:status 201,
             :body (json/json-str {
                                   :service_name service-name,
                                   :phone_number valid-phone-no,
                                   :status "active",
                                   :uri (:uri request)
                                        ; should have some kind of unique id for the phone number
                                        ; here.
                                        ; also return the date
                                   })})
          {:status 400,
           :body (json/json-str {:error "Please include a phone_number."})})))))

P.S. it's also good idea to check value of service-map - if it will nil, then you'll get NPE. So instead of (service-map :id) you can write (:id service-map) that will correctly work if service-map will nil...

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.