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 nil
s 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.