9

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.

Improve this question
26
  • 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. Commented Sep 18, 2011 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. Commented Sep 18, 2011 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. Commented Sep 18, 2011 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. Commented Sep 18, 2011 at 18:46
  • It's clever, but obtuse. Your call. Perhaps put a comment above the line explaining how it works. Commented Sep 18, 2011 at 18:49

7 Answers 7

Reset to default
14

"Good practice" - NO. As your peers may not be as intelligent. 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.

Keep it simple, even if it means a few more keystrokes.

Improve this answer
16
  • 10
    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. Commented Sep 19, 2011 at 1:16
  • 6
    @Raynos - I agree. If someone cannot understand that construct, they shouldn't be anywhere near your code... Commented Sep 19, 2011 at 3:22
  • 2
    If..Then..Else prevents fall through cases, especially with floating point numbers or unexpected input. Short circuit evaluation is harder to read, not just by other programmers but yourself later. Commented Sep 19, 2011 at 14:43
  • 2
    @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. Commented Sep 25, 2011 at 19:43
  • 2
    @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. Commented Sep 26, 2011 at 18:28
11

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.

Improve this answer
7
  • I agree with your point. Three lines, though. ;) Commented Sep 18, 2011 at 19:50
  • 1
    "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. Commented Sep 19, 2011 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) Commented Sep 19, 2011 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. Commented Sep 19, 2011 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". Commented Sep 19, 2011 at 12:11
8

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.

Improve this answer
1
  • 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. Commented Sep 19, 2011 at 0:26
8

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.

Improve this answer
1
  • 1
    '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. Commented Sep 24, 2011 at 5:19
1

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.

Improve this answer
7
  • ForceDirectories could still fail, so I do need to check again. And else if would not be executed after ForceDirectories was called. Commented Sep 18, 2011 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. Commented Sep 19, 2011 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. Commented Sep 19, 2011 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... Commented Sep 19, 2011 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. Commented Sep 19, 2011 at 3:47
0

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.

Improve this answer
4
  • 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.' Commented Sep 18, 2011 at 19:56
  • @Matthew Flynn - you forgot solution #3, applicable to the OP's construct: work with people who know something about programming. Commented Sep 24, 2011 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. Commented Sep 25, 2011 at 16:46
  • @Matthew Flynn - understood. My comment was a bit 'tongue in cheek'. Bills need to be paid.... Commented Sep 25, 2011 at 19:21
0

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.

Improve this answer
4
  • In theory DirExists is able to tell me more accurately whether ForceDirectories has succeeded than that function itself. Commented Sep 19, 2011 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. Commented Sep 19, 2011 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. Commented Sep 19, 2011 at 14:05
  • If at all possible, find a system function “create file or directory if it doesn’t exist” as an atomic function. Because otherwise files can be created by another thread or process between test and creating the file. Commented Jul 30 at 16:08

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.