if statement - short circuit evaluation vs readability

Question

Sometimes, an if statement can be rather complicated or long, so for the sake of readability it is better to extract complicated calls before the if.

e.g. this:

if (SomeComplicatedFunctionCall() || OtherComplicatedFunctionCall())
{
    // do stuff
}

into this

bool b1 = SomeComplicatedFunctionCall();
bool b2 = OtherComplicatedFunctionCall();

if (b1 || b2)
{
    //do stuff
}

(provided example is not that bad, it's just for illustration... imagine other calls with multiple arguments, etc.)

But with this extraction I lost the short circuit evaluation (SCE).

Do I really lose SCE every time? Is there some scenario where the compiler is allowed to "optimize it" and still provide SCE?
Are there ways of keeping the improved readability of the second snippet without losing SCE?

Practice shows that most answers about performance you will see here or on other places are in most cases wrong (4 wrong 1 correct). My advice is always do a profiling and check it yourself, you will avoid "premature optimization" and learn new stuff. — Marek R, Oct 17 '16 at 08:27
@MarekR is is not just about performance, it is about possible side effects in OtherCunctionCall ... — relaxxx, Oct 17 '16 at 08:45
Would this question be better suited to Programmers? It doesn't seem to be a code problem. — David, Oct 17 '16 at 09:29
@David when referring other sites, it is often helpful to point that [cross-posting is frowned upon](http://meta.stackexchange.com/tags/cross-posting/info) — gnat, Oct 17 '16 at 09:35
If readability is your primary concern, don't call functions with side effects inside of an if conditional — Morgen, Oct 17 '16 at 14:38
This is a style question. I have never seen a C++ style guideline that goes down to the level of detail that they need to explain breaking up long `if` conditions, but separate lines of the form `condition ||` is idiomatic in my experience. — QuestionC, Oct 17 '16 at 14:46
Potential close voters: read the question again. Part (1) is *not* opinion based, while part (2) can easily cease to be opinion-based through an edit that removes the reference to any supposed "best practice", as I am about to do. — duplode, Oct 17 '16 at 17:55
The questions have been quite adequately answered at this point. In my opinion, SCE is kind of a wart of the whole C language family. The order of execution in Fortran is not defined, and there is no really good reason why C could not have done the same. Relying on it makes the program weak with respect to future modifications by someone who did not appreciate that the SCE was intentional and gains nothing that the language did not already have. — Fred Mitchell, Oct 19 '16 at 00:58
If the question is about function calls which have side-effect, then you need to edit the title and details to say that, and how nasty the side-effects are. As stands this just looks like a generic style question. — smci, Oct 19 '16 at 08:14
@smci To me it looks exactly the opposite -- the side-effects are not essential to the question. `OtherComplicatedFunctionCall()` might be merely a computation with no side-effects that happened to take a very long time to complete, and yet the same concerns mentioned by the OP would still apply. — duplode, Oct 19 '16 at 16:25
@duplode: if the fn calls don't have side-effects then it reduces to a question about style and readability, which is more subjective, as people said. The performance argument is a red herring. It's up to the OP to clarify what they're asking about. — smci, Oct 19 '16 at 21:25
@smci It is clearly not a red herring. The OP says "But with this extraction I lost the short circuit evaluation". While the OP does not say why they care about short circuiting, there is absolutely no need for writing that explicitly, as skipping side-effects and skipping expensive computations are two plausible, legitimate and obvious reasons for wanting to use short circuiting. There is no need to state the obvious. — duplode, Oct 19 '16 at 21:33
We know they lost the SCE, but they still didn't state what the impact was (did it have side-effects and was it slow) and whether those were fixable. If they want us to suggest how to refactor `*FunctionCall()` so it returns earlier, then they'd need to tell us more about it. If all this grief is only for the sake of avoiding one long line containing the name `ComplicatedFunctionCall()`, then shorten that name already (or use a function pointer). — smci, Oct 20 '16 at 07:51

Horia Coman · Accepted Answer · 2016-10-17T10:05:43.803

119

One natural solution would look like this:

bool b1 = SomeCondition();
bool b2 = b1 || SomeOtherCondition();
bool b3 = b2 || SomeThirdCondition();
// any other condition
bool bn = bn_1 || SomeFinalCondition();

if (bn)
{
  // do stuff
}

This has the benefits of being easy to understand, being applicable to all cases and having short circuit behaviour.

This was my initial solution: A good pattern in method calls and for-loop bodies is the following:

if (!SomeComplicatedFunctionCall())
   return; // or continue

if (!SomeOtherComplicatedFunctionCall())
   return; // or continue

// do stuff

One gets the same nice performance benefits of shortcircuit evaluation, but the code looks more readable.

edited Oct 17 '16 at 10:05

answered Oct 17 '16 at 08:09

Horia Coman

8,681
2
23
25

but this apply only in the loops and methods... sometimes, there is more stuff to do after the `if` – relaxxx Oct 17 '16 at 08:11
4

@relaxxx: I get ya, but "more stuff to do after the `if`" is also a sign that your function or method is too large, and should be split into smaller ones. It doesn't always be the best way but very often is! – mike3996 Oct 17 '16 at 09:16
2

this violates the white list principle – JoulinRouge Oct 17 '16 at 09:21
@relaxxx I added a better approach than the initial one. I think this one captures what you were trying to achieve correctly and without being constrained in its usage – Horia Coman Oct 17 '16 at 10:07
13

@JoulinRouge: Interesting, I had never heard of this principle. I myself prefer this "short-circuit" approach for the benefits on readability: it reduces indentations and eliminates the possibility that something occurs after the indented block. – Matthieu M. Oct 17 '16 at 12:32
2

Is it more readable? Name `b2` properly and you would get `someConditionAndSomeotherConditionIsTrue`, not super meaningful. Also, I have to keep a bunch of variables on my mental stack during this exercise (and tbh until I stop working in this scope). I would go with `SJuan76`'s number 2 solution or just put the whole thing in a function. – Nathan Cooper Oct 17 '16 at 12:44
1

@relaxxx you could always replace the `return` with `goto` :^) – Soapy Oct 17 '16 at 14:02
I think `b1 = b1 || SomeOtherCondition();` would read better. Does `||=` short circuit? – Oct 17 '16 at 16:43
Used this many time, but with only 1 `bool` variable: `b = b || foo();` – chux - Reinstate Monica Oct 17 '16 at 18:26
what is bn_1? I assume its meant to be N conditions, but I do not see any of the normal signs that it does. – Ryan Oct 17 '16 at 18:32
@Hurkyl: There is no operator `||=` (or `&&=`) in C++, you have to use the more verbose form. And I agree with you just reusing the same variable is better in this case rather than numbering it. – Matthieu M. Oct 18 '16 at 10:50
2

I haven't read all the comments but after a fast search I didn't find a big advantage of the first code snippet namely debugging. Placing stuff directly into the if-statement instead of assigning it to a variable beforehand and then using the variable instead makes debugging more difficult than it needs to be. Using variables also allows to group values semantically together which increases readability. – rbaleksandar Oct 18 '16 at 18:18
Writing as the initial solution is highly beneficial since it makes the code [orthogonal](http://programmers.stackexchange.com/a/122625/11110). – hlovdal Oct 19 '16 at 11:44

AmigoJack · Answer 2 · 2016-10-20T06:27:23.307

I tend to break down conditions onto multiple lines, i.e.:

if( SomeComplicatedFunctionCall()
 || OtherComplicatedFunctionCall()
  ) {

Even when dealing with multiple operators (&&) you just need to advance indention with each pair of brackets. SCE still kicks in - no need to use variables. Writing code this way made it much more readible to me for years already. More complex example:

if( one()
 ||( two()> 1337
  &&( three()== 'foo'
   || four()
    )
   )
 || five()!= 3.1415
  ) {

score 28 · Answer 3 · answered Oct 17 '16 at 08:10

If you have long chains of conditions and what to keep some of the short-circuiting, then you could use temporary variables to combine multiple conditions. Taking your example it would be possible to do e.g.

bool b = SomeComplicatedFunctionCall() || OtherComplicatedFunctionCall();
if (b && some_other_expression) { ... }

If you have a C++11 capable compiler you could use lambda expressions to combine expressions into functions, similar to the above:

auto e = []()
{
    return SomeComplicatedFunctionCall() || OtherComplicatedFunctionCall();
};

if (e() && some_other_expression) { ... }

score 21 · Answer 4 · answered Oct 17 '16 at 08:10

1) Yes, you no longer have SCE. Otherwise, you would have that

bool b1 = SomeComplicatedFunctionCall();
bool b2 = OtherComplicatedFunctionCall();

works one way or the other depending if there is an if statement later. Way too complex.

2) This is opinion based, but for reasonably complex expressions you can do:

if (SomeComplicatedFunctionCall()
    || OtherComplicatedFunctionCall()) {

If it ways too complex, the obvious solution is to create a function that evaluates the expression and call it.

KIIV · Answer 5 · 2016-10-18T18:11:47.903

21

You can also use:

bool b = someComplicatedStuff();
b = b || otherComplicatedStuff(); // it has to be: b = b || ...;  b |= ...; is bitwise OR and SCE is not working then

and SCE will work.

But it's not much more readable than for example:

if (
    someComplicatedStuff()
    ||
    otherComplicatedStuff()
   )

edited Oct 18 '16 at 18:11

answered Oct 17 '16 at 08:22

KIIV

3,534
2
18
23

3

I'm not keen on combining booleans with a bitwise operator.That doesn't seem well typed to me. Generally I use whatever looks most readable unless I'm working very low level and processor cycles count. – Ant Oct 18 '16 at 16:21
3

I've used speciffically `b = b || otherComplicatedStuff();` and @SargeBorsch make an edit to remove SCE. Thanks for noticing me about that change @Ant. – KIIV Oct 18 '16 at 18:09

songyuanyao · Answer 6 · 2016-10-20T00:33:15.440

1) Do I really lose SCE every time? Is compiler is some scenario allowed to "optimize it" and still provide SCE?

I don't think such optimization is allowed; especially OtherComplicatedFunctionCall() might have some side effects.

2) What is the best practice in such situation? Is it only possibility (when I want SCE) to have all I need directly inside if and "just format it to be as readable as possible" ?

I prefer to refactor it into one function or one variable with a descriptive name; which will preserve both short circuit evaluation and readability:

bool getSomeResult() {
    return SomeComplicatedFunctionCall() || OtherComplicatedFunctionCall();
}

...

if (getSomeResult())
{
    //do stuff
}

And as we implement getSomeResult() based on SomeComplicatedFunctionCall() and OtherComplicatedFunctionCall(), we could decompose them recursively if they're still complicated.

i like this because you can gain some readability by giving the wrapper function a descriptive name (albeit probably not getSomeResult), too many other answers don't really add anything of value — aw04, Oct 19 '16 at 17:41

score 9 · Answer 7 · answered Oct 17 '16 at 08:10

1) Do I really lose SCE every time? Is compiler is some scenario allowed to "optimize it" and still provide SCE?

No you don't, but it's applied differently:

if (SomeComplicatedFunctionCall() || OtherComplicatedFunctionCall())
{
    // do stuff
}

Here, the compiler won't even run OtherComplicatedFunctionCall() if SomeComplicatedFunctionCall() returns true.

bool b1 = SomeComplicatedFunctionCall();
bool b2 = OtherComplicatedFunctionCall();

if (b1 || b2)
{
    //do stuff
}

Here, both functions will run because they have to be stored into b1 and b2. Ff b1 == true then b2 won't be evaluated (SCE). But OtherComplicatedFunctionCall() has been run already.

If b2 is used nowhere else the compiler might be smart enough to inline the function call inside the if if the function has no observable side-effects.

2) What is the best practice in such situation? Is it only possibility (when I want SCE) to have all I need directly inside if and "just format it to be as readable as possible" ?

That depends. Do you need OtherComplicatedFunctionCall() to run because of side-effects or the performance hit of the function is minimal then you should use the second approach for readability. Otherwise, stick to SCE through the first approach.

levilime · Answer 8 · 2016-10-18T16:54:42.923

8

Another possibility that short circuits and has the conditions in one place:

bool (* conditions [])()= {&a, &b, ...}; // list of conditions
bool conditionsHold = true;
for(int i= 0; i < sizeOf(conditions); i ++){
     if (!conditions[i]()){;
         conditionsHold = false;
         break;
     }
}
//conditionsHold is true if all conditions were met, otherwise false

You could put the loop into a function and let the function accept a list of conditions and output a boolean value.

edited Oct 18 '16 at 16:54

answered Oct 17 '16 at 14:15

levilime

362
1
9

1

@Erbureth No they aren't. The elements of the array are function pointers, theyy're not executed until the functions are called in the loop. – Barmar Oct 18 '16 at 18:50
Thanks Barmar, but I made an edit, Erbureth was right, before the edit (I thought my edit would propogate visually more directly). – levilime Oct 18 '16 at 19:18

score 4 · Answer 9 · edited Jul 21 '17 at 00:00

Very strange: you are talking about readability when nobody mentions the usage of comment within the code:

if (somecomplicated_function() || // let me explain what this function does
    someother_function())         // this function does something else
...

In top of that, I always preceed my functions with some comments, about the function itself, about its input and output, and sometimes I put an example, as you can see here:

/*---------------------------*/
/*! interpolates between values
* @param[in] X_axis : contains X-values
* @param[in] Y_axis : contains Y-values
* @param[in] value  : X-value, input to the interpolation process
* @return[out]      : the interpolated value
* @example          : interpolate([2,0],[3,2],2.4) -> 0.8
*/
int interpolate(std::vector<int>& X_axis, std::vector<int>& Y_axis, int value)

Obviously the formatting to use for your comments may depend on your development environment (Visual studio, JavaDoc under Eclipse, ...)

As far as SCE is concerned, I assume by this you mean the following:

bool b1;
b1 = somecomplicated_function(); // let me explain what this function does
bool b2 = false;
if (!b1) {                       // SCE : if first function call is already true,
                                 // no need to spend resources executing second function.
  b2 = someother_function();     // this function does something else
}

if (b1 || b2) {
...
}

score -7 · Answer 10 · answered Oct 17 '16 at 08:12

-7

Readability is necessary if you work in a company and your code will be read by someone else. If you write a program for yourself, it is up to you if you want to sacrifice performance for the sake of comprehensible code.

answered Oct 17 '16 at 08:12

br0lly

35
5

24

Bearing in mind that "you in six months time" is *definitely* "someone else", and "you tomorrow" can sometimes be. I would never sacrifice readability for performance until I had some solid evidence that there was a performance problem. – Martin Bonner supports Monica Oct 17 '16 at 09:33

if statement - short circuit evaluation vs readability

10 Answers10