Is it bad practice to use short-circuit evaluation instead of an if clause?

Question

I'm working on an installation script, and especially there I have many things that need to be checked and actions that only need to be performed in case of related outcomes. For example, a folder only needs to be created if it doesn't exist:

MyDesktopFolderPath := AddBackslash(ExpandConstant('{commondesktop}')) +
                         'My Folder';
if not DirExists(MyDesktopFolderPath) then
begin
  ForceDirectories(MyDesktopFolderPath); //Create full path
end;
if DirExists(MyDesktopFolderPath) then
begin
  //Perform other actions

Instead of this, I could use (exploit?) short-circuit evaluation to make it more compact:

MyDesktopFolderPath := AddBackslash(ExpandConstant('{commondesktop}')) +
                        'My Folder';
IsMyDesktopFolderExisting := DirExists(MyDesktopFolderPath) or
                               ForceDirectories(MyDesktopFolderPath);
if IsMyDesktopFolderExisting then
begin
  //Perform other actions

This would work too, but I wonder if it is bad practice, mostly because it is less obvious that actions might be performed. Any opinions on that?

Edit: As pointed out by Secure, the code is not completely equivalent, but the discussion topic still stands for other cases.

score 15 · Accepted Answer · answered Sep 18 '11 at 18:57

"Good practice" - NO. As your peers may not be as intelligent as you are. But yes, in some languages (I'd dare say, Perl), That is a common idiom. But "exploiting" short circuit evaluation - if it was considered good practice,

It certainly would have appeared to be a common idiom in a place like the Linux kernel source (Those guys swear to write the clearest code on earth).
If..Then..Else would have been done away with by now.

So, my suggestion, keep it simple, even if it means a few more keystrokes. Just an observation and a personal opinion, though.

score 11 · Answer 2 · answered Sep 18 '11 at 18:56

Short circuiting was added as a means to improve performance when evaluating a boolean expression. Using it as a form of control flow can lead to confusion many people will not expect evaluating an expression to have a side effect. In most cases, being more explicit about what you are doing is better for readability.

If you look at this example, exploiting short circuiting only saves you two lines. In my opinion, that is not enough to accept the potential issues that may arise from the implicit side effect. In order to prevent the confusion, you might add a comment to explain it. But you are not decreasing the line count savings. In the end, you need to perform a simple operation and by taking advantage of this feature, you decrease the clarity of the operation. You now have to add an explanation instead of letting the code speak for itself.

score 7 · Answer 3 · answered Sep 18 '11 at 22:15

Short-circuit evaluation is not just about performance. It's also about preventing run-time errors. For example, I would consider:

if (foo != null && foo.bar == 4) {
    // do something
}

to be perfectly clear and preferable to

if (foo != null) {
    if (foo.bar == 4) {
        // do something
    }
 }

Note that the first form only works if there is short-circuit evaluation... otherwise it generates a run-time error whenever foo == null.

score 7 · Answer 4 · answered Sep 19 '11 at 00:37

It's fine to me; this is a very common idiom in many languages: sh, C++, Perl, Lisp, Ruby, etc.

I would not add a comment; the comment would add nothing to the code.

I don't have much sympathy for colleagues who can't be bothered to parse any code that is slightly unfamiliar.

Vector · Answer 5 · 2011-09-19T02:30:04.133

This is Delphi-Pascal code - setting a boolean using 'or' could be considered obfuscation but I don't think any attentive, experienced developer would have any problem with it - I use this construct very frequently and I've never had a complaint.

However, I'd limit such a shortcut to one condition in most cases - if you string together a few conditions to get to one boolean value, you are definitely taxing the reader and introducing potential for nasty bugs.

As an aside, if I understand your code correctly, in second example, 'if IsMyDesktopFolderExisting then' is unnecessary: at that point the condition is always true.

Also: first example: 'if DirExists(MyDesktopFolderPath)' all you need here is 'else' - boolean is binary: it's either true or false. - if that's not what's supposed to be happening here, this code needs refactoring.

score 0 · Answer 6 · answered Sep 18 '11 at 19:24

0

If you think someone might be confused by a construct, there's two solutions:

Write a comment explaining what you did.
Write it in a less confusing manner.

If you find you're having to do #1 more often than you'd like, try #2.

answered Sep 18 '11 at 19:24

Matthew Flynn

13,495
2
39
58

score 0 · Answer 7 · answered Sep 19 '11 at 08:55

Your first version could as well be like this:

IsMyDesktopFolderExisting := DirExists(MyDesktopFolderPath);
if not IsMyDesktopFolderExisting then
begin
  IsMyDesktopFolderExisting := ForceDirectories(MyDesktopFolderPath);
end;
if IsMyDesktopFolderExisting then
...

While using a short-circuit tells me to look more carefully what's done here, ignoring the return value of a resource handling function (like memory or files) immediately raises a red flag.

Is it bad practice to use short-circuit evaluation instead of an if clause?

7 Answers7

Linked

Related