Nest the if statements then, perhaps in an else branch. Relying on short circuiting to avoid side effects or improving performance meaningfully is pretty shaky.
Says who? IME relying on short circuiting is useful and idiomatic in C and C++ code, e.g.:
if (callback && callback->run()) { ... }
if (callback) {
if (callback->run()) {
..
}
}
In a case like this, my preference would be to give callback a dummy value and always run it.
I subscribe pretty heavily to the Carmack philosophy of always running the code as similarity as possible and making a decision at the end of a procedure.
I'm curious, what leads you to believe that skipping unnecessary calls to functions with side effects is something to be avoided? Do you believe that unconditionally calling code with side effects specially when you don't have to is something to be desired?
I would almost always rather support default behavior through dummy data rather than null checks. I find that a lot of software bugs come from the unintended consequences of branching. Especially branches which create state that is consumed later.
In fact, I find that most null checks lead to bad software behavior as it often hides bugs. Early outting when null is detected is especially silly to me. The client has just tried to run a procedure which had to be abandoned due to a lack of data and you're just going to quit silently?
If you're using dummy data for default scenarios and asserting not null rather than checking, you will eliminate most logical null checks in your code. Though I do concede that in the case where you must support null checks, such a short circuit as above is acceptable and in fact preferable to me over two if statements.
In the above example, we check that the pointer is non-null before calling it. The "side effect" we are avoiding is a segfault.
In the code that splits the conditional into two, the callback is still run inside a conditional, just by itself.
in mine, yes definitely 100% the second code is more readable than the first
If anything, adding a nesting level for each chained predicate makes code harder to read, and needlessly so.
The issue is not verbosity. It's the needless addition of nesting levels and independent expressions in a non-idiomatic way, which greatly increase the cognitive load of checking a single row of chained predicates. It's bad code.
if(an expression) {
if(another expression) {
}
}
if(an expression && another expression) {
}
- did the author think about operator precedence
- did the author want to use && or &
- did the author think about short-circuiting (as evidenced in this very thread, a lot of people do not)
I question the idea that you would ever want the very linchpin of your function hidden away as a second subclause of an if statement. A function that calls callbacks is probably mainly doing that, let it stand proud and clear in the code text.
If your function is calling callbacks left and right “on its way somewhere else”, I think a refactor is in order.
Maybe you will now present some second scenario with something other than callbacks. Maybe in that case you’ll have me convinced it’s an okay exception. For you see, I said only it’s “pretty shaky”, not a mortal sin.
With that said though, I think in 9/10 cases, it’s something you can solve with an early return.
if (!callback)
return;
callback();You've got it all backwards. Short-circuiting is not used to avoid side effects. Short-circuiting is used to avoid needless calls to any predicate, which can have and often do has side-effects, and if it doesn't have today it might have tomorrow.
If you don't call them when you don't have to, your code is in a better shape.
And let's not pretend that unconditionally macode with side effects when you don't have to is a good practice.