views:

74

answers:

1

I'm looking for a more idiomatic way, if possible, to write the following clojure code:

(import '(System.Net HttpWebRequest NetworkCredential)
        '(System.IO StreamReader)) 

(defn downloadWebPage
  "Downloads the webpage at the given url and returns its contents."
  [^String url ^String user ^String password]
  (def req (HttpWebRequest/Create url))
  (.set_Credentials req (NetworkCredential. user password ""))
  (.set_UserAgent req ".NET")
  (def res (.GetResponse req))
  (def responsestr (.GetResponseStream res))
  (def rdr (StreamReader. responsestr))
  (def content (.ReadToEnd rdr))
  (.Close rdr)
  (.Close responsestr)
  (.Close res)
  content
  )

This is on ClojureCLR and works. (the fact that it's the CLR variant doesn't matter much)

I'd like to get rid of the defs (replace by lets? can they refer to each other?)

How about a better way to get to the stream - keeping in mind that .. chaining won't work because I need to Close the streams later on.

EDIT: After the answer, I found a much easier way in .NET to download a web page using the WebClient class. I still used many of Michal's recommended approaches - just wanted to record what I now believe to be the best answer:

(defn download-web-page
    "Downloads the webpage at the given url and returns its contents."
    [^String url ^String user ^String password]
    (with-open [client  (doto (WebClient.)
                        (.set_Credentials (NetworkCredential. user password "")))]
      (.DownloadString client url)))
+5  A: 

The code from the question can be rewritten fairly idiomatically like so (modulo any typos -- here's hoping there are none):

(defn download-web-page
  "Downloads the webpage at the given url and returns its contents."
  [^String url ^String user ^String password]
  (let [req (doto (HttpWebRequest/Create url)
              (.set_Credentials (NetworkCredential. user password ""))
              (.set_UserAgent ".NET"))
        response        (.GetResponse req)
        response-stream (.GetResponseStream res)
        rdr             (StreamReader. response-stream)
        content (.ReadToEnd rdr)]
    (.Close rdr)
    (.Close response-stream)
    (.Close response)
    content))

Assuming the .NET version of with-open calls .Close at the bound objects (as I expect it might, but won't be able to check -- no .NET REPL at hand) and that .readToEnd eagerly consumes the whole stream, this could be further simplified to

Update: Just checked that ClojureCLR's with-open calls .Dispose on the bound objects. If that is ok in place of .Close, great; if .Close is required, you can write your own version of with-open to use .Close instead (possibly copying most of the original):

(defn download-web-page
  "Downloads the webpage at the given url and returns its contents."
  [^String url ^String user ^String password]
  (let [req (doto (HttpWebRequest/Create url)
              (.set_Credentials (NetworkCredential. user password ""))
              (.set_UserAgent ".NET"))]
    (with-open [response        (.GetResponse req)
                response-stream (.GetResponseStream res)
                rdr             (StreamReader. response-stream)]
      (.ReadToEnd rdr))))

Some comments:

  1. Don't use def, defn etc. anywhere except at top level unless you really know you need to do that. (Actually using them immediately inside a top-level let is occasionally useful if you need the object being created to close over the let-bound locals... Anything more funky than that should receive very careful scrutiny!)

    def & Co. create top level Vars or reset their root bindings; doing so in the course of a programme's regular operation is completely contrary to the functional spirit of Clojure. Perhaps more importantly from a practical POV, any function which relies on "owning" a bunch of Vars can only be executed by one thread at a time; there's no reason why download-web-page should be thus limited.

  2. let-introduced bindings may not be mutually recursive; later bindings may refer to earlier bindings, but not the other way around. Mutually recursive local functions may be introduced with letfn; other types of mutually recursive objects may be somewhat less convenient to create outside of top level (though by no means impossible). The code from the question doesn't rely on mutually recursive values, so let works fine.

Michał Marczyk