Count Words in Quotes Fetched from Website

Question

I have solved the following exercise and would like to get some feedback on my implementation.

Create a function that uses futures to parallelize the task of downloading random quotes from https://www.braveclojure.com/random-quote using (slurp "http://www.braveclojure.com/random-quote"). The futures should update an atom that refers to a total word count for all quotes. The function will take the number of quotes to download as an argument and return the atom’s final value. Keep in mind that you’ll need to ensure that all futures have finished before returning the atom’s final value.

(defn quote-word-count
  [quote-count]
  (->> quote-count
      (range)
      (map (fn [_] (fetch-quote)))
      (pmap (comp count-words deref))
      (apply merge-with +)
      (atom)))

(defn fetch-quote
  []
  (future (slurp "https://www.braveclojure.com/random-quote")))

(defn update-word
  [word-counts word]
  (update word-counts word (comp inc zero)))

(defn zero
  [number]
  (if (= nil number) 0 number))

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (reduce update-word {} (clojure.string/split cleaned-quote #" "))))

(defn clean-input
  [input]
  ((comp remove-commas remove-author clojure.string/lower-case) input))

(defn remove-author
  [quote]
  (clojure.string/replace quote #".\n-- .*\n" ""))

(defn remove-commas
  [quote]
  (clojure.string/replace quote #"," ""))

Things I am not sure about:

1. the range then map to create the calls
2. the top-down order of functions
3. the atom. the exercise says to return an atom but I don't understand why I would need that

Any feedback is appreciated.

Answer 1

For quote-word-count, there's a few things to note.

I think this is a good use of ->>. Personally, I would have started the threading with (range quote-count) instead of splitting that though. From my experience, needlessly elongating the thread call just hurts readability. I also recommend maintaining the same type of object being threaded all the way down. I find it makes the thread much easier to understand. If you're threading collections like you are here, I'd prefer that every object being threaded is a collection (whereas quote-count is a number).

It isn't a good idea to use map to run side effects (like starting a future task). map doesn't run its effect immediately, so the futures aren't started when (map (fn [_] (fetch-quote))) runs, they're started when the pmap begins requesting elements. I'd switch to mapv which is strict, so the future's are started right away.

I don't think that mapping a range is the right approach here though; which is hinted at by your use of a wrapper (fn [_]) function. repeatedly would be a better candidate for this situation. It's lazy though, so I'd probably add a call to vec after it to realize it right away.

---

Your zero is actually unnecessary. fnil can be used here instead:

(update word-counts word (fnil inc 0)))

---

I wouldn't have parameters called quote ideally. quote is a rather important built-in, and it's a good idea to avoid shadowing built-ins.

---

I'd probably use ->> in count-words:

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (clojure.string/split cleaned-quote #" ")
         (reduce update-word {}))))

I personally find that to be much more readable than the nested version.

I'll note too that Clojure has the built-in frequencies that can do the work here:

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (clojure.string/split cleaned-quote #" ")
         (frequencies))))

---

In clean-input, I wouldn't use comp. I think it would be much neater using ->:

(defn clean-input
  [input]
  (->> input
       (clojure.string/lower-case)
       (remove-author)
       (remove-commas)))

I really only like comp when it's being passed to another function. Writing out ((comp g f) x) just seems like an overly complicated way of applying x to f then g.

---

Your functions are in reverse order for some reason. I'm not sure where you're running this that's allowing for functions to be used before they're declared.

---

Personally, I like having the parameter vector on the first line unless I have a doc string. I find the functions look noisy otherwise.

---

You're writing clojure.string/ a fair amount. You may want to require that module to avoid verbose, qualified function calls. While you're at it, you might as well add a ns macro call, since each file should really have one anyways:

(ns your.file.path.here
  (:require [clojure.string :as s]))

Now, clojure.string/split can be written as s/split.

---

And for 3.:

The function will take the number of quotes to download as an argument and return the atom’s final value.

It wants you to return the value of the atom, not the atom itself. They want you to update an atom inside the futures; although I don't think that's necessary, or even preferable.

---

---

Here's the final code I ended up with:

(ns your.file.path.here
  (:require [clojure.string :as s]))

(defn remove-author [quote]
  (s/replace quote #".\n-- .*\n" ""))

(defn remove-commas [quote]
  (s/replace quote #"," ""))

(defn clean-input [input]
  (->> input
       (s/lower-case)
       (remove-author)
       (remove-commas)))

(defn count-words [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (s/split cleaned-quote #" ")
         (frequencies))))

(defn fetch-quote []
  (future (slurp "https://www.braveclojure.com/random-quote")))

(defn quote-word-count [quote-count]
  (->> (repeatedly quote-count fetch-quote)
       (vec)  ; To ensure futures start before pmap begins processing chunks
       (pmap (comp count-words deref))
       (apply merge-with +)))