A pattern I particularly dislike: Function parameters which may or may not be collections.
Say you have a function that does some operation on a batch of inputs:
(defn process-batch [items] ;; ... do some work with items ... )
Say further that, for this process, the fundamental unit of work is always a batch. Processing one thing is just a batch size of one.
Lots of processes are like this: I/O (arrays of bytes), database APIs (transactions of rows), and so on.
But maybe you have lots of code that mostly deals with one thing at a time, and only occasionally makes a larger batch. In the name of “convenience,” people write things like this:
(defn wrap-coll "Wraps argument in a vector if it is not already a collection." [arg] (if (coll? arg) arg [arg])) (defn process "Processes a single input or a collection of inputs." [input] (process-batch (wrap-coll input)))
This is prevalent in dynamically-typed languages of all stripes. I think it’s a case of mistakenly choosing convenience over clarity.
This leads easily to mistakes like iterating over a collection, calling process on each element, when the same work could be done more efficiently in a batch.
Now imagine reading some code when you encounter a call to this function:
(process stuff)
Is stuff a collection or a single object? Who knows?
When you read code, there’s a kind of ad-hoc, mental type-inference going on. This is true regardless of what typing scheme your language uses. Narrowing the range of possible types something can be makes it easier to reason about what type it actually is.
The more general principle:
Be explicit about your types even when they’re dynamic.
If the operation requires a collection, then pass it a collection every time.
A “helper” like wrap-coll saves you a whopping two characters over just wrapping the argument in a literal vector, at the cost of lost clarity and specificity.
If you often forget to wrap the argument correctly, consider adding a type check:
(defn process-batch [items] {:pre [(coll? items)]} ;; ... )
If there actually are two distinct operations, one for a single object and one for a batch, then they should be separate functions:
(defn process-one [item] ;; ... ) (defn process-batch [items] ;; ... )
I really appreciate the clarity with which you’ve been describing these anti-patterns and their effects, as well as the solutions. I don’t often find myself in violation, but it certainly I can point some of the new-to-Clojure members of my team at.
Also, what are your thoughts on destructuring parameters?
I’ve heard somewhere that is is frowned upon, but in something like this:
(defn publish-field-change
"Publishes a change to a field, specifying its new value and new error message (which may be nil)."
[{:keys [data-key]} key value error]
It is more clear, I think, for a caller, a reader of the documentation, or someone testing the code, which key(s) in the first parameter are actually used. A little bit of Schema here would also go a long way towards identifying what that first parameter is in the larger context.
Thanks, Howard!
I have given some thought to destructuring map parameters, but not to the point where I can express it as a general principle. I don’t have all the answers, at least not yet. ;)
The one thing I like about seeing map de-structuring is that I know this arg will be a map, and some of the keys I can count on it having. I think it goes along with the sentiment of this post: be explicit even with dynamic types.
This is also the first of this series of posts, they are great Stuart! Looking forward to reading the backlog.
Howard – I often want to do exactly what you are doing, and have read the same opinions you have. It also causes me some cognitive dissonance not having a name to document the argument as a whole. My compromise has been to add the :as directive in the destructuring even if I don’t use it like so:
(defn publish-field-change
"Publishes a change to a field, specifying its new value and new error message (which may be nil)."
[{:keys [data-key] :as whatever} key value error]
This discussion has gone off the topic of the original post. If you would like to continue it, I suggest moving to the Clojure mailing list.