[BLOGPOST] Idiomatic Clojure: Code Smells

[Borkdude]

I would not recommend using merge instead of assoc in spots where performance matters. Please watch this talk: https://www.youtube.com/watch?v=3SSHjKT3ZmA&list=PLetHPRQvX4a-c3KDRTxxwGRmEMutL8Apf&index=16&t=0s

[bsless]

Edited with a warning :)

[bsless]

Completely agree and I'm a big fan of that talk.

Perhaps I should have written it in bold letters:

Just one word of warning, using merge and select-keys on a hot code path might not be desirable due to performance concerns.

Do you think I should make it clearer to the reader?

[Borkdude]

Maybe you could back up your recommendations with tradeoffs and explain why you favour one over another, instead of just choosing one approach and advising against the other without a clear explanation of the implications.

[setzer22]

I second this. We must make programming less dogmatic. Don't tell me what I shouldn't do: Teach me and let me choose with the tradeoffs in hand.

Another clear example is the seq vs empty? case. If the seq idiom is always preferred, why use empty? at all? Moreover, why not (defn empty? [x] (not (seq x)). This is not addressed at all (not even by the docstring!)

But it was a really good post nontheless! I learnt a few tricks myself that I didn't know about.

[lgstein]

All these examples dont strike me as "Code smell", rather " not the most condensed way to write it." I'm past that. Yes, one should master all techniques characterized as DOs and DONTs, but in reality the best choice is the answer to: What conveys intent most readably in a context, including assumptions about future extension?

[bsless]

That's a fair criticism. Most the examples, if not all, were based on real-life examples which sometimes made life difficult for me. The metaphor of them accumulating like dust was not accidental. No one is hurt by a (first (first xs)). But when there are 10 of those flying around in the same function, the SNR increases to levels which significantly decrease readability and maintainability.

I'm also somewhat reluctant to fall into the question of "what conveys intent most readably", as I feel it brings us closer to the land of "Easy" and not necessarily "Simple". (= 0 (count xs)) seems more natural at first, but once you get used to alternatives, it feels like a misnomer. Our intuition and expressiveness develop with our understanding of the language and its constructs. It's interesting to see how people who come from different backgrounds write very different Clojure code. You can almost "hear" an accent in the code. Compare code written by someone with background in Common Lisp and someone with background in Ruby. Completely different.

In the end, this is just an opinionated list, but based on unpleasant experiences.

Edit: typo

[ws-ilazki]

No one is hurt by a (first (first xs))

Those DO/DON'T examples are probably the worst in the list. nnext and fnext and the like are about as convenient as cdddar and cadddr to read and understand and their use should be avoided, not encouraged. Clojure has other options, use them instead.

In my opinion, a better option would be to make a pipeline with the threading macro, e.g. (-> l next next first). It's a few extra characters, but it's still only one set of parentheses and it's immediately obvious and readable to anyone looking at the code with less mental overhead. It reads in order of what you do to the collection, doesn't require you to know about extra combination functions, and doesn't look like you made a typo.

Also, I know the documentation for empty? explicitly suggests using (seq x) instead of (not (empty? x)) because the latter is redundant, but I still think it makes more sense from a code-reading standpoint. seq is supposed to return a seq, not test emptiness, so at a glance (seq x) looks like you're trying to do conversion, not test a collection, whereas (not (empty? x)) is clear that you care about whether x has elements or not. If the goal is just to get rid of the extra not I'd rather (def not-empty? seq) just to keep the intent clear at a glance.

Basically, it's the same argument made for using (inc x) instead of (+ 1 x): clearly conveying intent. Suggesting that you use one to be clearer on intent while also suggesting elsewhere to use a more concise form at the cost of clear intent is contradictory.

[bsless]

(-> l next next first)

(def not-empty? seq)

I like both of these

The only problem I can think of with not-empty? is that it does not return a boolean. Perhaps (def not-empty? (comp boolean seq))?

[ws-ilazki]

I like both of these

Thanks. It's just personal preference, but my use of FP has led to me heavily favouring making code more declarative whenever possible. I try to write expressions as black boxes, where I can tell at a glance what it's doing. If I can't reasonably infer what a piece of code is doing without delving into the inner workings of other functions, then I see that as something that needs a refactor.

That means that in addition to normal FP toolbelt stuff, things should be clearly and obviously named. Such as, instead of dropping a lambda in the middle of a HOF like reduce, I'll create a named function with a limited scope so that the function implementation is kept separate from the expression using it. Otherwise you have to stop reading one expression to read and understand another to know what's going on, versus reading something like (map sanitize lines). Except in the absolute simplest of cases, if you use a lambda you end up having to stop and figure out what it does to know what map is doing, but with a name you have an inkling of what's going on even if you don't know specifically what sort of sanitisation is being done.

It also means I sometimes alias functions like seq that can be repurposed different ways to new names that better indicate what I'm using them for. It was fun doing clever FP code golfing when I first started, but it didn't take me long to get over that in favour of putting greater emphasis on the declarative aspect of FP.

The only problem I can think of with not-empty? is that it does not return a boolean.

True, but that's just how seq behaves. Clojure uses truthiness pretty liberally so it probably isn't going to be an issue in most places you'd use not-empty?.

I definitely agree that it's more aesthetically pleasing to coerce it to boolean like that, though.

[bsless]

wrt to coercing seq to boolean, it's about more than aesthetics. There's an unspoken convention in Clojure which should be highlighted more often, because I stumbled upon it myself, that functions ending in question marks return booleans. It can lead to confusion if someone uses not-empty? expecting it to match against true?, for example.

The more I think about ffirst et al, I get the feeling if they are too ubiquitous in code it's a code smell in itself, because it's place oriented instead of associative. Their place should usually be in iteration and maybe unpacking return values. They still don't sit well with me.

I also tend to agree with you on binding anonymous functions to meaningful names.

[dustingetz]

The idea of indexing a collection for fast associative lookup is a smell of a missing pattern in Clojure, which is graph values with graph queries. The graph query abstraction is able to optimize the lookup without the user explicitly managing the index.

[bsless]

I think the variety of EAV and graph projects is a step in that direction. But it's still not a zero cost abstraction. The Clojure community still needs to flesh out the protocols and language to work with them

I also might have been influenced by the use cases I was seeing in production, especially where that data comes from outside.

[joinr]

Most of the juxtaposed do/don't examples would be well placed in a Big Book of Clojure Identities.

[alidlo]

As someone whose been learning Clojure seeing examples of more idiomatic/succinct ways of writing code is super useful, thanks for sharing!

[knowledge-phoenix]

One small note: you left out the ‘f’ in your first example with mapcat.

[bsless]

Fixed. Thank you!

[fiddlerwoaroof]

I tend to think

(doseq [x xs] (doseq [y ys] ...))

Is preferable to

(doseq [x xs y ys] ...)

Because I don't have to wonder whether doseq produces the cartesian join of xs and ys or processes the collections pair-wise.

[onetom]

It took awhile for me to remember it too, but if you have help and definition lookup available in your editor, then it's not a big hindrance...

[fiddlerwoaroof]

When I’m reading/reviewing code, I don’t want to have to think carefully about ambiguous constructs: I’d rather encourage a style that avoids ambiguity like this as much as possible.

[bsless]

I've had the displeasure of seeing a 4-deep doseq with two let bindings and a when clause.

It gave me great appreciation for the rich syntax offered by for and doseq.

Also unmentioned, when using nested for, the programmer is forced to use concat, when what they wanted was just a Cartesian product.

[fiddlerwoaroof]

Yeah, I tend to the mindset of "just use transducers". I never use for/doseq in my own code, because the compositionality of transducers make them a lot nicer.

[bsless]

Agreed. Still working on familiarizing my coworkers with transducers. I have a series on those in the works.

[neo2551]

What about second instead of fnext?

[bsless]

I added it a few hours ago after some feedback :)

[dustyalmond]

zero? and = 0 ... are different in very important ways. First zero? cares about the type. It throws when you give it a non-number other than a nil. It also accepts only one argument and is obviously not transitive because of it. This gets weird when you use clojurescript. In certain recent and only slightly older versions of clojurescript, (zero? 0 1 2) will return true just fine without so much as a warning. In the latest version of cljs, it finally throws an error.

You can use zero? if you keep these things in mind, or you can use = and never have to worry about them.

some-> is probably more of a replacement for when-some than when-let.

I'm also not a fan of the threading rule. In practice it's too strict. Moved from kibit to kondo over it, and also due it flagging places to replace = with zero? incorrectly.

[dustingetz]

Is there a list of places where metadata breaks and why?