11
\$\begingroup\$

Is there a way to write this better or more Clojure way? Especially the last part with with-open and the let. Should I put the -> form into a separate function? Is this function to overloaded (to much stuff) or is this still ok?

 (defn scrape-book-cover [url image-filename]
   (let [out-stream (io/output-stream image-filename)
         image (-> url
                   (java.net.URL.)
                   (html/html-resource)
                   (html/select [:div.bookcoverXXL :> :div :> :img])
                   (first) (:attrs) (:src)
                   (->> (str "http:"))
                   (http-client/get {:as :byte-array})
                   (:body))]
     (with-open [out out-stream]
       (.write out image))))
\$\endgroup\$
  • 2
    \$\begingroup\$ You have many forms with single element you can ignore parens around them 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\$ – abuzittin gillifirca Mar 20 '14 at 16:12
  • \$\begingroup\$ Thanks for the hint with the parens around single argument functions. The "http:" needs to be added because the url begins with "//" and clj-http can't deal with that (MalformedURLException) \$\endgroup\$ – Fu86 Mar 20 '14 at 18:19
9
\$\begingroup\$

@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))))
\$\endgroup\$
  • \$\begingroup\$ I have no knowledge in Clojure what so ever, but your answer are always well-presented and in a good format! Your advises seems always good! + 1 \$\endgroup\$ – Marc-Andre Mar 20 '14 at 19:27
  • 3
    \$\begingroup\$ Thanks! :) I was surprised people weren't doing more reviews of Clojure code on here, so I figured I'd make some contributions. \$\endgroup\$ – Dave Yarwood Mar 20 '14 at 19:34
  • \$\begingroup\$ Thanks a lot! This helps me understand a lot of other things as well! One little thing I've come up with the replacement of the ugly ->> form inside the -> was the as-> form: (as-> u (str "http:" u)). Is this more Clojure idiomatic? \$\endgroup\$ – Fu86 Mar 20 '14 at 21:24
  • \$\begingroup\$ I've never seen that before... is there documentation online somewhere? I think (#(str "http:" %)) is plenty idiomatic. I've read a few books on Clojure and I remember seeing that syntax used somewhere, maybe in The Joy of Clojure. I always find it kind of awkward deciding which threading macro to use, because I have to stop and think, "how many of these functions would I need to pass in the argument in the second position, and how many would I need to pass it in the last position?" I usually just kind of guess and then use (#(fn args %)) as needed. \$\endgroup\$ – Dave Yarwood Mar 20 '14 at 21:51
  • \$\begingroup\$ @DaveYarwood: See here: clojure.github.io/clojure/clojure.core-api.html#clojure.core/…> But you made a good point for using an plain old lambda for that. It is much more visible whats going on. \$\endgroup\$ – Fu86 Mar 20 '14 at 22:08

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.