Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
    
I don't know which language the code is from, but both seems perfectly fine. Besides if 'exploit?' would have been a better option than 'if', why it would have been added in the first place. –  Pankaj Upadhyay Sep 18 '11 at 18:29
    
@PankajUpadhyay: I mean that making use of short-circuit evaluation might be considered exploiting it, that is, using it for something it isn't meant for. –  thoiz_vd Sep 18 '11 at 18:33
    
I don't like your second way, because ForceDirectories produces side-effects; your code suggests that it returns a boolean instead. That will be confusing for the programmer that has to maintain this code after you. –  Robert Harvey Sep 18 '11 at 18:38
    
@RobertHarvey: That is what I also thought of, but the thing is, that it happens often that a boolean is assigned a value by a function that performs an action either successfully (True) or not (False). Only in this particular case I can check the result of ForceDirectories using DirExists again. –  thoiz_vd Sep 18 '11 at 18:46
    
It's clever, but obtuse. Your call. Perhaps put a comment above the line explaining how it works. –  Robert Harvey Sep 18 '11 at 18:49

7 Answers 7

up vote 10 down vote accepted

"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,

  1. 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).
  2. 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.

share|improve this answer
    
Though all answers are right, I found your answer most convincing, because it contains facts I hadn't thought of. –  thoiz_vd Sep 18 '11 at 19:55
7  
In many scripting languages this is good practice (JS/python/ruby). I would actively recommend doing this. It's not that confusing. If it is you should be teaching your programmers to program. –  Raynos Sep 19 '11 at 1:16
3  
@Raynos - I agree. If someone cannot understand that construct, they shouldn't be anywhere near your code... –  Vector Sep 19 '11 at 3:22
1  
@yati: I don't care if someone finds it confusing the first time. They will get used to it. Most novices don't stay novices, and I'm not writing code to accommodate the ones who do. –  kevin cline Sep 25 '11 at 19:43
1  
@kevin hey, you're taking this veryyy far :) It's not about knowing short ckt boolean exps, but about where to use them. Then probably you expect those modern language programmers to never use exception handling in C++/Python and let the maintainers/porters figure stuff out. Then comments should be very redundant for you, as an "Experienced" coder should always be able to interpret code. Then naming a class as DoThisStuffForMe (a verb) should also not be a problem. Using try..catch as "lazy" replacements for if..else should also be okay. Collaboration requires good programming practice. –  yati sagade Sep 26 '11 at 18:28

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.

share|improve this answer
    
I agree with your point. Three lines, though. ;) –  thoiz_vd Sep 18 '11 at 19:50
    
"Short circuiting was added [for] performance" - citation please? In C, it allows the common idiom if (p && p->q), i.e. it's not for performance but for correctness. –  MSalters Sep 19 '11 at 9:40
    
Found a source myself: it's also called McCarthy evaluation, after the LISP inventor who introduced them: dtic.mil/cgi-bin/…. See page 21, they're short circuited pretty much by accident (it follows from their definition) –  MSalters Sep 19 '11 at 9:59
    
@MSalters: The fact that trailing arguments do not have to be evaluated does not imply that they won't be evaluated. Skipping the trailing arguments is an optimization because it does not change the end result. Most modern languages support short circuiting, but not all. Your pointer dereference example is another example of exploiting short circuiting as a nested if statement provides the same correctness check, just more explicitly. –  unholysampler Sep 19 '11 at 11:34
    
@unholysampler: I quoted the LISP manual; in that context there's no ambiguity. They won't be evaluated. That's how the operators are defined in LISP. The SE question you link to only covers new languages (at least a decade younger), so they don't really cover "why short circuiting was [initially] added". –  MSalters Sep 19 '11 at 12:11

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.

share|improve this answer
    
The question is not whether relying on short-circuit itself is bad, but if one should use it for conditional execution like foo != null && foo.bar == 4 && DoSomething(), where DoSomething() only happens if the two statements preceding it are true. –  thoiz_vd Sep 19 '11 at 0:26

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.

share|improve this answer
    
'I would not add a comment; the comment would add nothing to the code.' +1 - comments are to explain what isn't obvious from the code itself - they aren't meant to serve as a tutorial in a language. The OP's construct is very elementary and generic. –  Vector Sep 24 '11 at 5:19

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.

share|improve this answer
    
ForceDirectories could still fail, so I do need to check again. And else if would not be executed after ForceDirectories was called. –  thoiz_vd Sep 18 '11 at 20:39
    
"ForceDirectories could still fail, so I do need to check again" - then maybe you need an exception handler, not a condition like that - semantically that code doesn't make much sense. –  Vector Sep 19 '11 at 2:31
    
I really don't see your problem with this. I start off with a situation in which I don't know if a folder exists, so I check it and try to have it created if it doesn't. After that, the situation may have changed (folder created) or not (folder still present or after all not created), so I check again. Just a simple way to be very certain that everything is how it should be, at a low cost - an exception handler would give lots of code overhead and exceptions should be avoided when they can effectively be replaced with conditionals. –  thoiz_vd Sep 19 '11 at 3:10
    
"exceptions should be avoided when they can effectively be replaced with conditionals" ? The whole point of exceptions is to avoid conditional checks... –  Vector Sep 19 '11 at 3:24
2  
Exceptions help avoid having to use lots of return codes for all kinds of very unlikely (exceptional) errors, but in my case there are only two options, so a boolean suffices. Besides, ForceDirectories is part of the framework I'm using, so to use exceptions, I would have to wrap it, resulting in more code, without getting more detailed error messages. –  thoiz_vd Sep 19 '11 at 3:47

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

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

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

share|improve this answer
    
So the question was rather how likely it is someone will be confused by this construct. But by now it seems that the common opinion is 'too likely.' –  thoiz_vd Sep 18 '11 at 19:56
    
@Matthew Flynn - you forgot solution #3, applicable to the OP's construct: work with people who know something about programming. –  Vector Sep 24 '11 at 5:22
    
@Mikey - often easier said than done--geography and world realities often get in the way. And the question is not just whether the current group of developers can understand your code, but whether the folks who need to update it in three years or migrate it in ten can. –  Matthew Flynn Sep 25 '11 at 16:46
    
@Matthew Flynn - understood. My comment was a bit 'tongue in cheek'. Bills need to be paid.... –  Vector Sep 25 '11 at 19:21

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.

share|improve this answer
    
In theory DirExists is able to tell me more accurately whether ForceDirectories has succeeded than that function itself. –  thoiz_vd Sep 19 '11 at 12:14
    
@thoiz_vd: Then I'd call it bad API design, because it violates the principle of least surprise. But wait, I don't know much Delphi, but Google returned that ForceDirectory returns true if the creation was successful, and DirExists may return false if the user is not authorised to see the dir. Create/Write and List/Read are different permissions in most file systems. But then your second version in the OP is not equivalent to your first version, it has a subtle difference in functionality. "This would work the same" is a wrong assumption. –  Secure Sep 19 '11 at 13:45
    
You turn out to be right about that, but it also means that the whole DirExists call can go, because ForceDirectories returns True even if the Folder already existed and thus wasn't created. Nevertheless, the topic was never about this particular example, but about using the structure in general, or in cases where the two forms are equivalent, if you prefer. –  thoiz_vd Sep 19 '11 at 14:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.