Push Ifs Up and Fors Down

Can I float an adjacent model? Classes are nouns, functions are verbs.

perfectly good models

Doesn't this obfuscate what assumptions you can make when trying to understand the core logic? You prefer to examine all the call chains everywhere?

You’ve kind of answered your own question here.

> If you push that conditional out to the caller, now every caller of your function has to individually make sure they call it in the right way to get a guarantee of idempotency

In this situation your function is no longer idempotent, so you obviously can’t provide the guarantee. But quite frankly, if you’re having to resort to making individual functions implement state management to provide idempotency, then I suspect you’re doing something very odd, and have way too much logic happening inside a single function.

Idempotent code tends to fall into two distinct camps:

1. Code that’s inherently idempotent because the data model and operations being performed are inherently idempotent. I.e. your either performing stateless operations, or you’re performing “PUT” style operations where in the input data contains all the state the needs to be written.

2. Code that’s performing more complex business operations where you’re creating an idempotent abstraction by either performing rollbacks, or providing some kind of atomic apply abstraction that ensures partial failures don’t result in corrupted state.

For point 1, you shouldn’t be checking for order of operations, because it doesn’t matter. Everything is inherently idempotent, just perform the operations again.

For point 2, there is no simple abstraction you can apply. You need to have something record the desired operation, then ensure it either completes or fails. And once that happens, ensures that completion or failure is persistent permanently. But that kind of logic is not the kind of thing you put into a function and compose with other operations.

Maybe write the functions without the checks, then have wrapper functions that just do the checks and then call the internal function?

The way to solve this is to split decisions from execution and that’s a notion I got from our old pal Bertrand Meyer.

    if (weShouldDoThis()) {
        doThis();
    }

Code scanners reports should be treated with suspicion, not accepted as gospel. Sonar in particular will report “code smells” which aren’t actually bugs. Addressing these “not a bug” issues actually increases the risk of introducing a new error from zero to greater than zero, and can waste developer time addressing actual production issues.

IME this is frequently a local optimum though. "Local" meaning, until some requirement changes or edge case is discovered, where some branching needs to happen outside of the loop. Then, if you've got branching both inside and outside the loop, it gets harder to reason about.

It can be case-dependent. Are you reasonably sure that the condition will only ever effect stuff inside the loop? Then sure, go ahead and put it there. If it's not hard to imagine requirements that would also branch outside of the loop, then it may be better to preemptively design it that way. The code may be more verbose, but frequently easier to follow, and hopefully less likely to turn into spaghetti later on.

(This is why I quit writing Haskell; it tends to make you feel like you want to write the most concise, "locally optimum" logic. But that doesn't express the intent behind the logic so much as the logic itself, and can lead to horrible unrolling of stuff when minor requirements change. At least, that was my experience.)

I've always hated code complexity scanners ever since I noticed them complaining about perfectly readable large functions. It's a lot more readable when you have the logic in one place, and you should only be trying to break it up when the details cause you to lose track of the big picture.

There was a thread yesterday about LLMs where somebody asked "what other unreliable tool people accept for coding?"

Well, now I have an answer...

> printInvoice(invoice, options)

The function printInvoice should print an invoice. What happens if an invoice cannot be printed due to one of the named conditionals being false? You might throw an exception, or return a sentinel or error type. What do to in that case is not immediately clear.

Especially in languages where exceptions are somewhat frowned upon for general purpose code flow, and monadic errors are not common (say Java or C++), it might be a better option to structure the code similar to the second style. (Except for the portrait format of course, which should be handled by the invoice printer unless it represents some error.)

> while code readability is extremely important, encapsulation is much more important

Encapsulation seems to primarily be a tool for long-term code readability, the ability to refactor and change code locally, and to reason about global behavior by only concerning oneself with local objects. To compare the two metrics and consider one more important appears to me as a form of category error.