current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

I am trying to find all "link" elements that link to RSS feeds in a data structure created by clj-tagsoup. I wrote the following code, which seems to work fine. But: I come from a Java background and I am new to functional programming and clojure, so I am not sure if this is the right way to do it. Could you please give me some hints about what I could improve in the following code:

(defn- matches-attributes [current-attributes required-attributes]
    (reduce #(and %1 (= (second %2) ((first %2) current-attributes)))
            true 
            required-attributes))

(defn- find-tag [tag-name current-tag attributes]
    (if (= tag-name (tagsoup/tag current-tag))
        current-tag
        (filter #(and (= tag-name (tagsoup/tag %1)) 
                (matches-attributes (tagsoup/attributes %1) attributes))
                (tagsoup/children current-tag))))

(defn- find-rss-feed [page-result]
    (let [document (tagsoup/parse-string (:body page-result))
          head (first (find-tag :head document nil))
          rss-links (if (nil? head) 
                        nil 
                        (find-tag :link head {:type "application/rss+xml"}))]
    ... do something with rss-links...))

Edit To clarify a bit, what I am particularily interested in is if traversing the HTML tree the way I do it is the "right" was in a functional language. What I'm also interested in is if "filter" and "reduce" are a good choice as I use them, or if there is a better way. And, of course, I'd also appreciate general hints about what I could do better in this little program...

share|improve this question

1 Answer 1

up vote 3 down vote accepted

It's not bad, but we can make it a bit more idiomatic. Let's look at this function first:

(defn- matches-attributes [current-attributes required-attributes]
    (reduce #(and %1 (= (second %2) ((first %2) current-attributes)))
            true 
            required-attributes))

The use of reduce is a bit odd, instead let's use every? Secondly, if current-attributes is a hash map or a set, then we can actually use it as a function. Since every? takes a predicate, we can rewrite the entire function as:

(defn- matches-attributes [current-attributes required-attributes]
  (every? (fn [tv tg] 
              (= (current-attributes tg) tv))
          required-attributes))

On the last function, when-let will help you clean it up a bit more: 

(defn- find-rss-feed [page-result]
    (let [document (tagsoup/parse-string (:body page-result))
          rss-links (when-let [head (first (find-tag :head document nil))]
                        (find-tag :link head {:type "application/rss+xml"}))]
    ... do something with rss-links...))

Hopefully this helps a bit. Your code doesn't really support this, but the cool thing to remember is that Clojure collections are functions that return nil if the item doesn't exist. So for example:

=> (remove #{1 2 3} [1 2 3 4 5 6 7])
(4 5 6 7)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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