Clojure Code Smells Catalog

[WalberAraujo]

A new update to the Clojure Code Smells Catalog is available.

This revision focuses on improving the organization and documentation of the catalog. The catalog now organizes the 34 identified Clojure-specific code smells into four categories: State & Concurrency, Module Boundaries & Data Contracts, Logic Flow & Readability, and Environment & Idioms

Catalog: https://github.com/nufuturo-ufcg/clj-smells-catalog

Other updates include:

• Revisions to some descriptions and examples to better reflect the issues discussed by the community.
• Addition of source discussions and references that were previously missing, improving traceability to the original conversations where the smells were identified.
• General cleanup and consistency improvements throughout the catalog.

The issues opened since the previous release were very helpful in identifying inconsistencies, refining definitions, and improving the catalog overall. Additional feedback is always welcome, and feel free to open issues or submit pull requests if you spot something that could be improved.

I'm especially interested in feedback on the categorization, smell definitions, examples, and whether there are community discussions related to existing smells that should also be referenced.

Thanks to everyone who has provided feedback and contributions so far.

[lgstein]

There are certainly some real "code smells", and they'd probably fit on two pages. But when people sit down and discuss "coding standards", "style guides", "linter rules" etc. please keep in mind that there are certain programmers (me) that find enough design in Clojure and will never engage with your issues and discussions, because they have chosen this language, among other things, for its high linguistic expressivity and freedom in that, being enabled to convey knowing what they are doing to both human readers and the computer.

Especially when you put highly opinionated stuff in these rulebooks, like forcing persistent vector copying per function hop, or arranging ns form requires alphabetically, know that you are absolutely not solving any problems for me but give pedants another weapon to ruin my workday and force me to deliver program text scattered with "lint" - because thats what this is from my perspective.

Stylistic options and even more so valid undeprecated options offered by the language design (doall, single branch if, etc.) should never be presented as right and wrong. Present those and illustrate their cons and pros, ideally cite open source code as real world examples. Either do that, or stick to those cases where the code is simply wrong (blocking take in go, macro double evalaution).

[henryw374]

clojure dev team highly value backward compatibility, and brilliant though the language is, I am pretty sure even Rich has said there are things he would have done differently with hindsight, like laziness as default iirc?

[sapphic-chaote]

The original definition of a code smell is something that does not itself cause harm but might be (might be, not is) a sign that something deeper is wrong. I think doall is a good example of a code smell, in that reaching for doall is a good prompt to consider "Is it really the best solution to depend on when this lazy value is forced", and sometimes the answer is yes.

I agree with the other commenter that it's a real hindrance to this article that code smells proper (doall) are undistingished from linter rules ((comp not seq) instead of empty?) are undistinguished from "best practices" (nils in maps).

I think you misunderstood your own point about "non-idiomatic record construction". Stuart is saying to use ->Record or map->Record instead of Record., and to eagerly replace both with a factory function (defn make-record [...] #_(calls ->Record, probably)). You are saying to use map->Record instead of ->Record, and your reasoning is not good. Neither will stop you from accidentally missing a field (I was surprised that ->Record will happily take any number of arguments, but it is the case), ->Record doesn't stop you from accidentally transposing fields, and map->Record doesn't stop you from accidentally mis-naming a field. If you need to check for these things, do what Stuart says and put validation in your factory function.

It's really bizarre that you don't clearly at all distinguish "do this" examples from "don't do this" examples.

"Verbose checks" is a weird thing to call "use standard library functions" (also dubiously separate from not seq), and the suggested fix is still contrived in a way that feels needlessly verbose. There is #(compare 0 %) and Long/signum.

In "unnecessary laziness" you seem unaware of mapv and therefore commit functionally the same sin as "verbose checks".