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.

thoiz_vd
  • 131
  • 2
  • 6
  • 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
  • LOL ..sorry i misread into....actually you used exploit?, so i thought it's somewhat like C# Null-Coalescing Operator...BTW which language are we talking about ? – Pankaj Upadhyay Sep 18 '11 at 19:02
  • @RobertHarvey: What other ways would I have to determine if, for example, PrintPage was successful than to write IsPagePrinted := PrintPage? I can only think of supplying it with a modifiable variable - PrintPage(ResultCode), but the less parameters the better, wasn't it? So in that way, side-effects are daily practice anyway, or am I not seeing something? – thoiz_vd Sep 18 '11 at 19:33
  • @PankajUpadhyay: This is Pascal, but the mechanism we're talking about applies to any language where expressions are no longer evaluated when the outcome is already certain. In C# you have a choice (& vs. && and | vs. ||). – thoiz_vd Sep 18 '11 at 19:34
  • One other way to do it (excuse my C#, but...) is `if (PrintPage()) { // do something }` -- This eliminates the need for the `IsPagePrinted` variable. – Robert Harvey Sep 18 '11 at 20:00
  • @RobertHarvey: I think it might be useful to be able to check the outcome once again without having to print another one, but yes, then it would become `if PrintPage() { IsPagePrinted = True; }`. Maybe it's just Pascal's (optional) BEGIN/END that makes it look bulky. – thoiz_vd Sep 18 '11 at 20:05
  • BTW, why not `DesktopFolderExists`, instead of `IsDesktopFolderExisting` ? – kevin cline Sep 19 '11 at 00:39
  • @kevincline: That's a debate I keep on having with myself; I try to stick with some boolean naming conventions (start with Is, Has, Can, and so on), even though I sometimes end up with a name that seems a bit off, like this. – thoiz_vd Sep 19 '11 at 03:16
  • @Pankaj : it comes from LISP, and dates back to at least 1962. http://www.dtic.mil/cgi-bin/GetTRDoc?AD=AD0406138&Location=U2&doc=GetTRDoc.pdf – MSalters Sep 19 '11 at 10:00
  • I don't understand why it's called 'short circuiting' and I don't understand why it's something that needs special introduction for 'performance' - it's **simple** boolean logic. – Vector Sep 19 '11 at 16:09
  • @thoiz_vd: Instead of following some rule, I try to make the code readable as English, so "isTall", but "hasChildren", "eatsHay", etc. This can't be done in Java, where property accessors must begin with "get" or "is". – kevin cline Sep 22 '11 at 19:02
  • @Mikey: it's called short-circuiting because if the first expression (for the `or` operator) is true, the second isn't evaluated. This means that `x or y` is not the same as `y or x`, even though in **simple** boolean logic, they are the same. – kevin cline Sep 24 '11 at 02:28
  • @kevin cline - not sure I follow you here: in the expression: 'bool x = ( (y==1) or (z==1) )' it's irrelevant whether y, z or both == 1 because x==true regardless - your expression doesn't concern itself with how the conclusion was reached - whether via x,y or both: ( (y==1) or (z==1) ) is a 'black box'. – Vector Sep 24 '11 at 05:12
  • @Mikey: Commonly it may not be necessary or possible to evaluate the second expression when the first is true: `DirectoryExists("x") or MakeDirectory("x")` – kevin cline Sep 24 '11 at 16:36
  • Not sure if you're agreeing with me or adding something I overlooked: My point is: "it may not be necessary or possible to evaluate the second expression " - who cares? The expression says that you DON'T care. – Vector Sep 24 '11 at 16:47
  • @Mikey - I think you are overlooking something. Often we DO care, e.g. `x != null and x.length > 3`, where the second expression will raise an exception when x is null. – kevin cline Sep 25 '11 at 06:04
  • "x != null and x.length > 3" - IMO that's not solid code at all - not appropriate use of the construct. If you need to know something about x, you must **first** validate it - ensure that it's not null - not wrap that validation condition into a boolean expression for use in the subsequent code. I'd write it like this: assert(x!=null,'x is null you dummy!'); bool y=(x.length>3) – Vector Sep 25 '11 at 06:21
  • @Mikey -- but it may be fine for x to be null. This is so common that several languages have a conditional member operator like `?.` – kevin cline Sep 25 '11 at 15:55
  • Understood - if it's 'fine for x to be null', then don't use an assertion. Regardless, if (x.length > 3) will blow up if x is null, that's not the right construct - it should be handled separately. Booleans should only be linked together when you KNOW every expression in your chain will indeed return a boolean value. If it doesn't, either fix it so that it does, or don't use it in a 'short circuit' context - address it separately. – Vector Sep 25 '11 at 19:30
  • @Mikey: I have found that such rigidity is impractical. These days short-circuit expressions are widely used in many languages, and often the clauses are not even boolean: e.g. `value = userValue || defaultValue` – kevin cline Sep 25 '11 at 19:39
  • let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/1432/discussion-between-kevin-cline-and-mikey) – kevin cline Sep 25 '11 at 19:39
  • @kevin cline - no time to chat now. We're in agreement that there's nothing objectionable about the 'short circuiting' example of the OP - it's routine. As for all the particulars of how and when to use such constructs, it depends on the context - I'm not an idealogue either. Cheers. – Vector Sep 25 '11 at 21:20

7 Answers7

15

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

yati sagade
  • 2,089
  • 2
  • 19
  • 27
  • 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
  • 9
    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 01:16
  • 5
    @Raynos - I agree. If someone cannot understand that construct, they shouldn't be anywhere near your code... – Vector Sep 19 '11 at 03:22
  • 1
    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. – Michael Shopsin Sep 19 '11 at 14:43
  • @Michael: it's like any other notation. It becomes familiar with practice. – kevin cline Sep 24 '11 at 16:40
  • @kevin even your regexes are familiar to *you*, the original coder. Think about the poor souls who might be required to maintain your code. – yati sagade Sep 24 '11 at 19:02
  • @yati sagade: Do you believe everything you read? Even if someone documented a regex, I would still have to read the code to see if the implementation matched the comment. OTOH, it does help when languages have a regex syntax that allows a regex to be broken into multiple lines, or split into named sub-expressions. There is great value in a readable presentation. – kevin cline Sep 25 '11 at 16:04
  • @kevincline Obviously!! I read my code! this time you're right! See my point was that a person from a C/C++/Java/Python background will find these constructs confusing. OTOH, people from Perl should find it reasonable. It all depends upon what language "cult" you are from. That's what I wanted to say :) – yati sagade Sep 25 '11 at 19:16
  • 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
  • @kevin I care. Most of us do. I envy your position - you *can* dare not to care!! really! – yati sagade Sep 25 '11 at 20:17
  • "See my point was that a person from a C/C++/Java/Python background will find these constructs confusing" ??? I've never written a line of Perl in my life - but I use this construct in Object Pascal, C++ and C# all the time. I understand that you are in a position where you need to deal with others who might have trouble with this construct- but I pity you! – Vector Sep 25 '11 at 21:25
  • Pity me not, as I am happy using If..else for my conditionals. As for whether what you do "all the time" is good practice or not(well, *that* was the question), now completely is upon you. some people may be used to using no indentation in C/C++/C#, some may be used to not checking a returned pointer.. But that doesn't make it good practice. As an example of a person you'll seriously want to pity, I give you Linus Torvalds, who happens to demand a strict coding style for the Linux kernel sources. And that man deals with a *lot* of people's code. – yati sagade Sep 25 '11 at 22:41
  • @yati: the problem is that practice leads to 100,000 line applications written in FORTRAN-4 instead of a 10,000 line application written in a modern language. Eventually such projects require armies of maintainers if they can be maintained at all. If the team can't handle short-circuited boolean expressions, what are they going to do with chains of C++ STL calls? Python list comprehensions? Groovy builders? – kevin cline Sep 26 '11 at 14:40
  • 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
  • @yati: you started with "... will find these constructs confusing". My position is that they need to get over it and learn this common idiom. I have seen this attitude; I worked on a team that preferred 20 lines of complex nested loops to three STL calls. I was there when the application died prematurely when the cost to maintain it exceeded the value of any new feature. – kevin cline Sep 26 '11 at 22:23
  • What you are suggesting *is* the right way to go, when talking about learning the seriously neglected STL and nullptr deref. *that* use of short ckt eval is not only readable, but really lifesaving. But I am talking about common Perl idioms like: "send_that_file() || go_to_hell();" which can be written much neatly in C/C++ using conditionals. The OP asked whether or not "Exploiting" short ckt eval to produce compact code is okay. So, according to my previous comment, there are a *lot* of things that may be exploited to get compact code. – yati sagade Sep 27 '11 at 05:04
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.

unholysampler
  • 7,813
  • 1
  • 30
  • 38
  • 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 09:40
  • Found a source myself: it's also called McCarthy evaluation, after the LISP inventor who introduced them: http://www.dtic.mil/cgi-bin/GetTRDoc?AD=AD0406138&Location=U2&doc=GetTRDoc.pdf. See page 21, they're short circuited pretty much by accident (it follows from their definition) – MSalters Sep 19 '11 at 09: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](http://stackoverflow.com/q/1232603/210526). 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
  • @MSalters: The creators of Lisp decided to make short circuiting part of the language spec rather than an implementation detail. The point of the link was to show that not everyone made the same decision. Honestly, I don't have a citation to prove that performance was the reason. All I can say is that early on, people were doing whatever they could to improve performance and storage space because they were so limited. Adding short circuiting is not a huge step to understand why it works. In my opinion, that is a more compelling argument than removing the need for a nested if. – unholysampler Sep 19 '11 at 12:47
  • @unholysampler actually, sometimes, short ckt evaluation is slower than usual and at other times, it's never better. (why slower? -> parallelism). The most realistic use in my case was to avoid null ptr deref in C/C++. :) – yati sagade Sep 19 '11 at 19:03
7

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.

JoelFan
  • 7,025
  • 4
  • 38
  • 53
  • 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 00:26
7

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.

kevin cline
  • 33,608
  • 3
  • 71
  • 142
  • 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. – Vector Sep 24 '11 at 05: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.

Vector
  • 3,180
  • 3
  • 22
  • 25
  • `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 02: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 03: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 03:24
  • "an exception handler would give lots of code overhead" - ?? How? Why? "Exceptions are a programmer's best friend" – Vector Sep 19 '11 at 03:27
  • 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 03:47
  • I disagree - IMO a good exception heirarchy is one of the most important components of solid, fail-safe code and they are not only for 'very unlikely' errors. The fact that most modern languages support building sophisticated exception handling frameworks supports that idea. Correct use of exceptions can save hundreds of lines of code and accompanying bugs, and gives you complete control over your execution paths and recovery mechanisms. But if you're working within a framework that makes such a model unviable, you don't have much choice. – Vector Sep 24 '11 at 05:32
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.

Matthew Flynn
  • 13,345
  • 2
  • 38
  • 57
  • 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 05: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
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.

Secure
  • 1,918
  • 12
  • 10
  • 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