3
\$\begingroup\$

I would like to write a little pet project using Clojure to study it, something like Stack Overflow client.

Here is a my first code to get list of new questions from site. I am in the very beginning in Clojure so any help will be appreciated.

core.clj:

(ns stackify.core
  (:require [clj-http.client :as client]
            [clojure.data.json :as json]
            [stackify.utils :as utils]
            [stackify.configuration :as config]))

; TODO: move key to environment variables
(def ^:const stackKey "put-your-key-here")
(def ^:const baseUrl "https://api.stackexchange.com/2.2/questions?page=%d&pagesize=100&fromdate=%d&order=asc&sort=creation&site=%s&key=%s")

; Fetch questions
(defn fetch-json [url]
  "fetch internet page by url and return JSON"
  (let [data (client/get url)
        body (get data :body)]

    (json/read-str body)))

(defn fetch-questions [page from site accum]
  "recursive function to get questions"
  (let [url (format baseUrl page from site stackKey)
        json-content (fetch-json url)
        questions (get json-content "items")
        has_more (get json-content "has_more")]

    (println (format "Fetching page %d (has_more=%b)" page has_more))

    (cond
      (= has_more true) (fetch-question (+ page 1) from site (concat accum questions))
      :else (concat accum questions))
    ))

(defn get-questions []
  (let [from (config/read-time)
        now (utils/current-time)]

    (println from)
    (config/save-time now)
    (fetch-questions 1 from "stackoverflow" (vector))))

(defn print-questions [questions]
    (doseq [q questions] (println (get q "title"))))

utils.clj:

(ns stackify.utils
  (:require [clj-time.core :as time]
            [clj-time.coerce :as tc]))

(defn current-time []
  "returns unix time"
  (let [moment (tc/to-long (time/now))]
    (int (/ moment 1000))))

(defn parse-int [s]
  "convert string to int"
  (Integer. (re-find  #"\d+" s )))

configuration.clj:

(ns stackify.configuration
  (:require [stackify.utils :as utils]))

(def ^:const configFileName "last_sync_time")

(defn save-time [value]
  "save value to configuration file; file will be overwritten"
  (spit configFileName value))

(defn read-time []
  "read last sync time from configuration file"
  (let [value (slurp configFileName)]
    (utils/parse-int value)))

(defn init []
  "initiliza configuration file"
  (save-time (utils/current-time)))
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

This is nice code for someone just learning Clojure. You haven't made any of the common pitfalls like trying to use def to create local variables. I'm going to just jump around and mention things as I see them.


Your parse-int function is much slower (and arguably more manual) than it needs to be.

If you run lein check on your project, you'll see the following warning:

call to java.lang.Integer ctor can't be resolved.

It's also highlighted in yellow in IntelliJ for the same reason. It can't tell what overload of Integer. to use since it doesn't know with certainty what type re-find returns. This is forcing it to use reflection at runtime to do lookups, which can be very slow. Add a type hint to tell it exactly what re-find returns:

(defn parse-int [s]
  "convert string to int"
  (Integer. ^String (re-find  #"\d+" s)))

Here's some timings using Criterium, a great benchmarking library. Below, cc is an alias for criterium.core:

; Without type hints

(cc/bench
  (parse-int "1234567890"))

Evaluation count : 38179800 in 60 samples of 636330 calls.
             Execution time mean : 1.636166 µs  ; <-----------------------
    Execution time std-deviation : 226.156823 ns
   Execution time lower quantile : 1.459082 µs ( 2.5%)
   Execution time upper quantile : 2.309723 µs (97.5%)
                   Overhead used : 4.213229 ns

Found 5 outliers in 60 samples (8.3333 %)
    low-severe   5 (8.3333 %)
 Variance from outliers : 82.3868 % Variance is severely inflated by outliers


; With type hints

(cc/bench
  (parse-int "1234567890"))

Evaluation count : 187133520 in 60 samples of 3118892 calls.
             Execution time mean : 318.835981 ns  ; <-----------------------
    Execution time std-deviation : 3.399965 ns 
   Execution time lower quantile : 314.452726 ns ( 2.5%)
   Execution time upper quantile : 326.997655 ns (97.5%)
                   Overhead used : 4.213229 ns

Found 4 outliers in 60 samples (6.6667 %)
    low-severe   2 (3.3333 %)
    low-mild     2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

That's over 5x faster just from adding type-hints.

Here's a personal favorite of mine though that's much faster, and just falls back onto Java's parseInt implementation:

(defn my-parse-int [^String str-num]
  (try
    (Integer/parseInt str-num)  ; Fall back onto Java

    (catch NumberFormatException _
      nil)))  ; Return nil on a bad parse

(cc/bench
  (my-parse-int "1234567890"))
Evaluation count : 1531312920 in 60 samples of 25521882 calls.
             Execution time mean : 35.208889 ns  ; <-----------------------
    Execution time std-deviation : 1.018581 ns
   Execution time lower quantile : 34.554978 ns ( 2.5%)
   Execution time upper quantile : 38.078887 ns (97.5%)
                   Overhead used : 4.213229 ns

Found 7 outliers in 60 samples (11.6667 %)
    low-severe   4 (6.6667 %)
    low-mild     3 (5.0000 %)
 Variance from outliers : 15.8023 % Variance is moderately inflated by outliers

35 ns!

Note how I'm wrapping the parsing call in a try and returning nil in the event of an exception. In Clojure, it's common for nil to represent the absence of a valid value; like after a bad parse. It's similar to other language's notion of an Optional type. This concept allows you to do all sorts of nice things. Like, say you have a list of possible String numbers and you want to grab the first valid number:

(some my-parse-int ["bad" "also bad" "123" "another bad"])
=> 123

some ignores all the nils because they're falsey.


(cond
  (= has_more true) (fetch-question (+ page 1) from site (concat accum questions))
  :else (concat accum questions))
))

Isn't written ideally.

  • A cond with only one condition should just be an if unless you have a strong suspicion that you'll need to add more cases later

  • (= ... true) is redundant. if is already checking if the condition evaluates to a truthy value.

  • (+ page 1) can be simply (inc page)

  • Trailing closing braces like that aren't generally considered to be idiomatic style. Ideally you're using a good IDE that uses par(enthesis)-infer. That lets you write Clojure code using indentation to indicate nesting like it's Python. I never worry about braces when writing Clojure, and never manually type closing braces.

    If you have a decent computer, I recommend the Community version of IntelliJ, paired with the Cursive plugin that allows for writing Clojure (both are free for personal use). I've been using this setup for three years now, and it's excellent (heavy, but excellent).

Anyways, I'd write that chunk just as

(if has-more
  (fetch-question (inc page) from site (concat accum questions))
  (concat accum questions))

That reads nicer anyways. You write (concat accum questions) twice though. I'd probably bind that to a name to reduce redundancy:

(let [...
      more-questions (concat accum questions)]
  ...

  (if has-more
    (fetch-question (inc page) from site more-questions)
    more-questions))

With

(defn print-questions [questions]
    (doseq [q questions] (println (get q "title"))))

I wouldn't shove everything onto one line like that. I would spread that out:

(defn print-questions [questions]
  (doseq [q questions]
    (println (get q "title"))))

I'm not sure if you know about Clojure's doc-strings, but you have yours in the wrong place. The doc-string comes before the argument list. defn actually expects a String to internalize to act as a doc-string (see the docs of defn). Just move it up:

(defn save-time
  "save value to configuration file; file will be overwritten"
  [value]
  (spit config-file-name value))

Now good IDE's can show that documentation when requested.

Also, most of your casing is correct, but remember Clojure using dash-casing like I changed it to, not camelCasing, or snake_casing like you're using elsewhere. Consistency is key for readability.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for advice. \$\endgroup\$
    – ceth
    Commented Jul 20, 2019 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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