Search code examples
error-handlingrefactoringcommon-lisp

While refactoring this Common Lisp code snippet, how to succesfully remove the cond clause wrapping a handler-case (error handling) situation?

I am using SBCL, Emacs, Slime, and Dexador (a library for HTTP requests). I have this function that works:

(defun old-handle-response-and-status (final-url method &optional user-content)
  (let ((status-code)
        (response))
    (cond ((equal method "get")
           (multiple-value-bind (bresponse bstatus-code)
               (handler-case (dex:get final-url)
                 (dex:http-request-bad-request ()
                   (values nil
                           "The server returned a failed request of 400 (bad request) status."))
                 (dex:http-request-failed (e)
                   (values nil
                           (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
             (list (setf response bresponse)
                   (setf status-code bstatus-code))))
          ((equal method "post")
           (multiple-value-bind (bresponse bstatus-code)
               (handler-case (dex:post final-url
                                       :content user-content)
                 (dex:http-request-bad-request ()
                   (values nil
                           "The server returned a failed request of 400 (bad request) status."))
                 (dex:http-request-failed (e)
                   (values nil
                           (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
             (list (setf response bresponse)
                   (setf status-code bstatus-code)))))))

It works for GET, POST, and the error handling works as expected when a HTTP request faces errors. The iconic examples to show it working are:

CL-USER> (old-handle-response-and-status "http://www.paulgraham.com" "get")

("<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\">
<html>
(big HTML omitted)
</html>"
 200)

CL-USER> (old-handle-response-and-status "https://httpbin.org/post" "post" '(("name" . "pedro")))
("{
medium JSON omitted
}
"
 200)

CL-USER> (old-handle-response-and-status "https://httpstat.us/409" "get")
(NIL "The server returned a failed request of 409 status.")

Ok. While refactoring this code, I was trying to remove the cond clause. Thus I did a new shorter version:

(defun new-handle-response-and-status (method-call)
  (let ((status-code)
        (response))
    (multiple-value-bind (bresponse bstatus-code)
        (handler-case method-call
          (dex:http-request-bad-request ()
            (values nil
                    "The server returned a failed request of 400 (bad request) status."))
          (dex:http-request-failed (e)
            (values nil
                    (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
      (list (setf response bresponse)
            (setf status-code bstatus-code)))))

It majorly works, but only when the request is successful:


CL-USER> (new-handle-response-and-status (dex:get "http://www.paulgraham.com"))
("
HTML omitted
</html>"
 NIL)

CL-USER> (new-handle-response-and-status (dex:post "https://httpbin.org/post" :content '(("name" . "pedro"))))
("{
medium JSON omitted
}
"
 NIL)

When the request is a failed HTTP request, the refactoring does not work as expected! When calling:

CL-USER> (new-handle-response-and-status (dex:get "https://httpstat.us/409"))

The Slime Debugger throws:

An HTTP request to "https://httpstat.us/409" returned 409 conflict.

I was expecting:

(NIL "The server returned a failed request of 409 status.")

I tried tweaking the input to be a quoted s-expression and inserting an eval:

(defun new-handle-response-and-status (method-call)
  (let ((status-code)
        (response))
    (multiple-value-bind (bresponse bstatus-code)
        (handler-case (eval method-call)
          (dex:http-request-bad-request ()
            (values nil
                    "The server returned a failed request of 400 (bad request) status."))
          (dex:http-request-failed (e)
            (values nil
                    (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
      (list (setf response bresponse)
            (setf status-code bstatus-code)))))

It works:

CL-USER> (new-handle-response-and-status '(dex:get "https://httpstat.us/409"))
(NIL "The server returned a failed request of 409 status.")

But, it feels as a bad practice - not really compatible with a refactoring effort. Is there a way to fix this without using eval?

Maybe using funcall?

Solution

  • The problem is that you're calling dex:get or dex:post before invoking the function, so the handler binding is not in effect.

    You need to pass a function that calls it, and then call that function.

    (defun new-handle-response-and-status (method-call)
  (let ((status-code)
        (response))
    (multiple-value-bind (bresponse bstatus-code)
        (handler-case (funcall method-call)
          (dex:http-request-bad-request ()
            (values nil
                    "The server returned a failed request of 400 (bad request) status."))
          (dex:http-request-failed (e)
            (values nil
                    (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
      (list (setf response bresponse)
            (setf status-code bstatus-code)))))

(new-handle-response-and-status (lambda () (dex:get "https://httpstat.us/409")))

    Or you could convert it to a macro:

    (defmacro new-handle-response-and-status (method-call)
  `(let ((status-code)
         (response))
     (multiple-value-bind (bresponse bstatus-code)
         (handler-case ,method-call
           (dex:http-request-bad-request ()
                                         (values nil
                                                 "The server returned a failed request of 400 (bad request) status."))
           (dex:http-request-failed (e)
                                    (values nil
                                            (format nil "The server returned a failed request of ~a status." (dex:response-status e)))))
       (list (setf response bresponse)
             (setf status-code bstatus-code)))))