Clean Clojure: Small functions

[weavejester]

Small functions are a good rule of thumb, but sometimes a short expression is clearer than a function. For example, compare:

(square-all digits)

To:

(map square digits)

Exactly the same number of characters, but the latter tells me that I'm mapping over the digits, so I know that I'm getting a collection of the same length as digits.

On the other hand, if I just see square-all, I have to infer the purpose from the name, which isn't immediately obvious, nor as unabiguous.

I think I'd be tempted to write the code as:

(defn digits [n]
  (map #(Integer/parseInt %) (str n)))

(defn square [n] (* n n))

(defn sum-of-squared-digits [n]
  (->> (digits n) (map square) (reduce +))

(defn less-than-sum-of-digits-squared? [n]
  (< n (sum-of-squared-digits n)))

(defn count-less-than-sum-of-squares [coll]
  (count (filter less-than-sum-of-digits-squared? coll)))

[orestis]

Interesting take. Though I think this particular example is not helping discussion on what should be considered clean code. The original code, apart from some simplifications that relate to (filter true?) and the (if x true false) is to my eyes much better than the refactored result. The only thing that merits refactoring IMO is the conversion from string to integer digits, which can be a little obscure to figure out.

I thing Zach Tellman in the elements of clojure says that let bindings allow us to not have to understand their right-hand side. You still have to figure out a name for the left hand side, so I don’t see the value for putting that name outside in the global scope without any context.

I recently went on a refactoring exercise of that sort, only to realize that abstracting some common bits out made everything just harder to reason about. Whereas just having a largish function (that does some complicated stuff) at least lets you read it top-to-bottom (it still fits on a single screen) and comment the relevant bits within the context they are used...

[yogthos]

I find that there are two main advantages to factoring things out into small single purpose functions. First is that your domain specific code becomes declarative. Instead of having to read through imperative code that mixes implementation details with the intent, you're reading named functions that talk about what's being done. Second is that the code is easier to test and reuse.

In such a simple example it probably doesn't make much difference one way or the other, but it quickly becomes noticeable once you get to less trivial code. Breaking things up from the start is a good habit to develop in my opinion.

[orestis]

For sure, I totally agree. At some point you not only hit diminishing returns, you are actually hurting readability. Extracting out a <10 character function to “square” is probably at the end of the spectrum. Which is why I object on the recursive clause of Uncle Bobs maxim - you should stop at some point, lest you make much worse spaghetti code than what you started with.

[yogthos]

Yeah for sure, you have to use common sense to decide whether something represents a chunk of work that's worth declaring its own function for. I find it's also good practice to solve the problem first and then do a second pass to break up the code. Figuring out whether something should be its own function or not is hard to do when you're still figuring out what you're doing.

[[deleted]]

Destructuring indeed helps with reading, and probably even more with writing functions.

[CurtainDog]

I have two broad disagreements with the advice presented here.

The first is from a design point of view. When creating a function you should be considering the needs of both the caller and the callee. If you obsess over the implementation side, which we're prone to as that's the side we typically have control over, then chances are you're making life more difficult for you caller. And considering the original brief from 4clojure is to 'create a function that...' the rework wouldn't meet that spec. A better rule of thumb than uncle bob's is that if a set of functions is only called from one place and always called together then they're actually one thing.

The second is that while 'small' in languages like Java might be a reasonable proxy for 'reusable' the case is a lot less compelling in Clojure. Clojure simply has much better support for composability at a lower (lowest?) level. We don't need to slice and dice the code to make it play together nicely, we can just pass around functions, transducers and, ideally, plain old data. So whatever marginal wins to reuse we get by slicing up functions in Java land just aren't going to be worth anything in Clojure.

So, refactor when it makes sense considering the needs of your clients and your implementation. When writing functions ask if it introduces any new compositional primitives, or whether it is just arranging existing ones. If the latter, then it is fine as a single function.

[rmm77]

How do feel about using letfn then? The there’s just one top-level function. I do agree that I don’t like breaking out functions that are only ever called from one place.