Code Health: simple ways to avoid complexity

If you have used CodeScene's Virtual Code Review or PR integrations, then you already know that complexity is a frequent target of CodeScene's Code Health penalties. 

Just one or two extra if / then blocks around or inside a function will quickly lead to a warning on a PR.

Some conditionals are directly related to the task at hand, to the business logic. Others are more "accidental", determined by language or implementation details. In Clojure, checking for nil can be one of those details.

Consider this example.

A service we consume produces a string, with key-value pairs. The pairs are separated by semi-colons; each key and value are separated by a colon:

status:1203;statusText:pending;startIndex:4309

Let's say we want to check the status. We write some code to parse the string, splitting on ; and then on :. And finally returning the status code.

Eventually, we realize that we probably want all the keys, so we write a function to extract everything into a map:

(defn parse-message [message-str]
  (let [kv-pairs (str/split message-str #";")]
    (->> kv-pairs
         (map #(str/split % #":"))
         (into {}))))

But what if the string is empty? What if it's not even there, and we have nil instead? Clojure's standard clojure.string/split throws an exception on nil.

Execution error (NullPointerException) at java.util.regex.Matcher/getTextLength (Matcher.java:1769).
Cannot invoke "java.lang.CharSequence.length()" because "this.text" is null

Our reflex is to add a defensive check:

(when message-str
  ( ;; parse our stuff)
   ))

Then we realize that we're maybe passing along a nil ourselves, which will force the caller to deal with that too.

So this, then?

(if message-str
  ( ;; parse our stuff)
   )
  {}) ;; or pass on an empty map

(Let's assume for the purpose of this example that we don't care about reporting errors: if the input is bad we just want an empty map.)

There's a second check we need to perform as well though. What if kv-pairs doesn't contain... pairs?

user> (parse-message "not pairs")
Execution error (IllegalArgumentException) at user/parse-message (form-init14991840198581339031.clj:187).
Vector arg to map conj must be a pair

(if (and message-str (message-str-is-valid? message-str))
  ( ;; parse our stuff))
  {}) ;; or pass on an empty map

We're safe now, right?

Sort of.

We've avoided a null pointer exception but our code is now nested inside a conditional. In this simple case it wouldn't matter much, but in more complex situations this additional complexity would indeed make the function harder to read. "What's that extra check…? Oh yeah, the nil." If you add enough of these, CodeScene will start to complain about complexity on your PR. We're also potentially passing a nil onto the caller, who will have to deal with the same problem, one way or another.

What if we could just rewrite clojure.string/split to change the way it handles nil? Well… we can: that's what fnil is for.

If you don't remember fnil, it's a core Clojure function that wraps a function and basically intercepts a nil argument and replaces it with whatever default you provide.

That gives us this:

(defn parse-message [message-str]
  (let [kv-pairs ((fnil str/split "") message-str #";")]
    (->> kv-pairs
         (map #(str/split % #":"))
         (into {}))))

To solve the second problem, with the key value pairs, we can just filter out anything that's not a pair:

(defn parse-message [message-str]
    (let [kv-pairs ((fnil str/split "") message-str #";")]
      (->> kv-pairs
           (map #(str/split % #":"))
           (filter #(= (count %) 2))
           (into {}))))