6
\$\begingroup\$

Examples of where I've started migrating to short-circuit evaluation:

PHP

$id=0;//initialized in case of no result
$r=mysql_query(SQL);
if($r && mysql_num_rows($r)>0){
  list($id)=mysql_fetch_row($r);
}

becomes

$id=0;
$r=mysql_query(SQL);
$r && mysql_num_rows($r)>0 && list($id)=mysql_fetch_row($r);

JavaScript

var Req=jQuery.ajax(OBJECT);
//Something has happened and now i want to abort if possible
if(Req && Req.abort){
  Req.abort();
}

becomes

var Req=jQuery.ajax(OBJECT);
//Something has happened and now i want to abort if possible
Req && Req.abort && Req.abort();

I'm familiar with the short if style of

if(Req && Req.abort) Req.abort();

but it feels clunkier than short-circuit evaluation.

\$\endgroup\$
  • \$\begingroup\$ This question may have been acceptable at the time it was asked, but it does not meet Code Review's question standards in 2015. Since you've asked about two isolated excerpts in two different languages that have nothing to do with each other besides the use of &&, this is a generic best-practices question, and therefore off-topic. \$\endgroup\$ – 200_success May 10 '15 at 20:08
8
\$\begingroup\$

This is an abuse of short-circuit evaluation. It's not immediately obvious what the intent is. Sure, i can eventually see that you're getting an ID, calling Req.abort(), whatever. But i shouldn't have to decipher a line of code first to figure out what it does. Conciseness never trumps readability in source code. (Sure, minifying/packing JS has its benefits. But you don't write packed/minified code; you start out with decent code and then run a tool on it.)

Since your intent is to do the one thing if the other stuff is true, the code should say that.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The added benefit is that when you see if (x) { y }, you can read x or y quickly and decide if you need to read the full statement or can skip it because it doesn't apply to your current task. With a single statement, you must read x plus enough of y to parse it. It makes scanning the code much harder. \$\endgroup\$ – David Harkness Jul 1 '14 at 21:14
1
\$\begingroup\$

Also, jsbeautifier will format your code like this:

var Req = jQuery.ajax(OBJECT);  //<- Note the spaces around the equal sign
//Something has happened and now i want to abort if possible
if(Req && Req.abort)
{
  Req.abort();
}

Beyond, that lowerCamelCase should apply so Req -> req or even better request and OBJECT -> object, I am not too excited about either generic name, but I guess this is just a stub.

Furthermore it seems a bit paranoid to not trust request or request.abort(), I don't have enough code to see if this paranoia is justified.

Personally, I have taken to write this as :

( request && request.abort ) ? request.abort() : undefined;

or, with jQuery providing noop

( request && request.abort ) ? request.abort() : $.noop();

The undefined yells at my subconscious that I should really handle request or request.abort being falsey.

Furthermore, on if statements with 1 statement in the true block

Not acceptable:

if(Req && Req.abort) Req.abort();

Somewhat acceptable:

if(Req && Req.abort) 
  Req.abort();

Best practice:

if(Req && Req.abort){
  Req.abort();
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ If the coding standard were to allow single-statement blocks without braces, I would prefer to keep it in the same line to minimize the chance of adding a second statement without also adding braces. That said, I vastly prefer requiring braces in all cases for safety. \$\endgroup\$ – David Harkness Jul 1 '14 at 22:53
  • \$\begingroup\$ Hmmm, I never thought of it that way \$\endgroup\$ – konijn Jul 2 '14 at 12:30

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