short-circuits && clean code

I think it comes down to idiom.

Idioms in programming are miniature patterns that are commonly used. If there's more than one way to do something, one is normally the preferred idiom, either in the language community or else the project you're working in.

For example, you might see this in Perl:

fopen(my $fh, "filename") or die "Cannot open file";

That's a shortcut for an if statement, there - but that construct is very often used for error handling. Sometimes for default values. Rarely for anything else. This seems like a reasonable guide.

Also, I'd forgotten Perl, when I came to write that.

Following idioms is useful for two reasons. Firstly, it makes code more readable to one versed in common practise. Secondly, it has often been arrived at my lots of people finding out what worked effectively and what didn't.

errno ? fprintf(stderr, "Error: %s\n", strerror(errno)) : 0;

C has the ternary operator as well. If you ever do code like this I will hunt you down. It's possible to write this code (I think), and it's not a short-circuit (more on that later). But it's absolutely not idiomatic. The ternary operator is only ever used as part of an expression (and usually the expression itself), most often in a function argument list.

values = [x if x < 4 for x in arr] or None

You can do some pretty odd things with Python shortcuts. Here, selecting everything lower than 4 in an array, and if there's nothing then return None (a NULL-like object). Again, good for default values.

test -f /etc/passwd || echo "Password file missing!" >&2

Assuming I still recall shell programming, that's a fairly common shortcut idiom there, too, for error handling.

But I don't think you want to confuse this with short-circuit evaluation. Yes, this certainly relies on short-circuits to work, but you can (and absolutely should) use short-circuit eval always:

if (flag && expensive_operation()) {
  // Do something
}

That's only running the expensive operation if flag is set. Now, assuming that expensive_operation() has no side-effects - and it shouldn't, if we're following good practise - then putting these terms the other way around would have the same behaviour, but it'd run slower.

So short-circuits are fine, and to be encouraged. Shortcuts based on them are a matter for idiom, and should be used sparingly.

It appears that it's a growing idiom in JS. It's likely adapted from the similar Perl idiom.

Neither JSLint nor JSHint like it.

I'm personally of two minds: it's super succinct and clever. Once you've see it once it's pretty obvious what's going on.

On the other hand, it is conflating expressions and statements. Turning an expression into flow-control.

I would use it in the same way I use in-line if blocks: only use if there's one statement and it all fits in one line. I probably wouldn't do this in a professional code base, but for my own stuff, sure.

In short, if it saves you an indent/block and can fit in one-line, it's okay. Other useful purposes are:

I like to read this as the "...if x AND then y" version of x && y

isWizard && hat = getPointyHat();
validInput || haltAndCatchFire(); // error validation, reads well "is validInput or error"
!valid && haltAndCatchFire(); // doesn't read as well but same logic

It should be used to make simple blocks that would otherwise be:

if (x) {
    z = y();
}

into just x && z = y();

It shouldn't be used to handle any significantly complex logic or statements.

I'd say keep it limited to things like argument validations.

I'm with Evan Oman on this one: kill that sample snippet with fire.

And I suspect you already have an opinion of your own, or this idiom wouldn't have prompted you to post the question in the first place. ;)

But I think there's something going on here that's more interesting than arguing about which syntax is "best practice."

When writing your code, think about who will be reading it. Do you work on a team that's growing, or are you trying to attract contributors to your open source project? Then adopting highly idiomatic code will be counterproductive. It makes it harder for newcomers to get up to speed.

There's an old Brian Kernighan quote about debugging:

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.

I think the same principle applies to readability. The hard part of coding isn't the syntax, and it isn't reading individual lines or expressions. So being "clever" in how you write individual expressions doesn't achieve very much. It makes the code harder to read, and does nothing for the overall structure of the project.

Just from quickly glancing at it 😵, I think it should be used as sparingly as possible. I can see it being an ideal use-case in limited circumstances though.

My answer after reading the title: sure, short-circuiting is a handy tool sometimes.

My answer after reading that snippet: dear god kill it with fire.

I think short-circuiting is fine as long as it is still performing a normal boolean operation -- they should never be used to run arbitrary code.

I would avoid anything more complicated than something like

boolean fileIsValid = file.exists() && !file.isEmpty()

That kind of code is 'clever code'.

Clever code is only good for the person who writes it (or rather, their ego). For everyone else, boring code tends to be the best code in the world, because when it comes to code ... you do not want to be surprised.

In my opinion, the only few places where those are justified are variable declarations, e.g.

const myFunc = function (options) {
    options = options || {};
    const debugLevel = /debuglevel=(\d+)/.test(location.href) && RegExp.$1;
    // ...
}

Otherwise rather write the long variant in development code and use a minifier for production code, you'll end up using the shorter version while still having the longer one for ease of maintenance.