@abuzittingillifirca made a good point in that you can leave the parentheses off of the ->
forms that are just a single element.
html/html-resource
is actually quite flexible; it can take a variety of different kinds of arguments, including strings, URLs, input streams, etc. Assuming the url
argument to your function is a string, you can take out the java.net.URL.
part and just feed url
directly into html/html-resource
. In fact, I would begin the ->
part like this:
(-> (html/html-resource url)
...
So (html/html-resource url)
is like the "first step" in the threading macro. Less steps makes your code appear simpler and easier to read, just as long as the steps aren't overly complicated.
For the (first) (:attrs) (:src)
part, you might consider using get-in. This function comes in handy for navigating nested associative structures like vectors and maps -- it's perfect for something like enlive. You could re-word this part of your code as:
(get-in [0 :attrs :src])
It isn't much shorter than the original, but I think it reads better, and it "feels" shorter because you're only calling one function.
It's probably not very Clojurian to have a ->>
macro among the steps to a ->
macro. If you have a situation like this, where one of the steps to the ->
requires you to put the argument last instead of second, you can do something like (#(str "http:" %))
-- create an anonymous function literal with the %
wherever you want the argument to go, and then call it by putting parentheses around it.
So, here is how I would refactor your code:
(defn scrape-book-cover [url image-filename]
(let [out-stream (io/output-stream image-filename)
image (-> (html/html-resource url)
(html/select [:div.bookcoverXXL :> :div :> :img])
(get-in [0 :attrs :src])
(#(str "http:" %))
(http-client/get {:as :byte-array})
:body)]
(with-open [out out-stream]
(.write out image))))
EDIT: To answer your question: No, I don't think this function is overloaded. When you simplify like I did above, it doesn't seem too long or complicated.
EDIT 2: Of course, there's nothing stopping you from pulling out the ->
part and making it a separate function. It might make your code easier to read. You could call the function something like imgurl->bytes
.
(defn imgurl->bytes [url]
(-> (html/html-resource url)
(html/select [:div.bookcoverXXL :> :div :> :img])
(get-in [0 :attrs :src])
(#(str "http:" %))
(http-client/get {:as :byte-array})
:body))
(defn scrape-book-cover [url image-filename]
(let [out-stream (io/output-stream image-filename)
image (imgurl->bytes url)]
(with-open [out out-stream]
(.write out image))))
Come to think of it, I like this better. If you look at a lot of Clojure code on github, you will see a lot of things broken up into simple helper functions. I suppose it is generally idiomatic in Clojure to break things up in helper functions.
EDIT 3 (5/16/14): Someone just upvoted my answer, which made me remember this code and have a quick re-visit. You could actually further refactor this version of the scrape-book-cover
function by taking out the let
statement:
(defn scrape-book-cover [url image-filename]
(with-open [out (io/output-stream image-filename)]
(.write out (imgurl->bytes url))))
java.net.URL. html/html-resource .... first :attrs :src
instead of(java.net.URL.) (html/html-resource) .... (first) (:attrs) (:src)
. Why do you add"http:"
at the beginning of src attribute of the img element? \$\endgroup\$"http:"
needs to be added because the url begins with"//"
and clj-http can't deal with that (MalformedURLException
) \$\endgroup\$