Replacing IF with a logical expression in PHP

Question

I was refactoring some old code when I stumbled upon a construct similar to this:

// function bar() returns a value
// if the value is an instance of customException class, terminate with error code
// else process the regular data

$foo = bar();
checkForException($foo) && exit($foo->errorCode());
process($foo);

Now strange as it might seem, this is a lot shorter then

$foo=bar();
if(checkForException($foo)) {
    exit($foo->errorCode();
}
else {
    process($foo);
}

And somewhat more readable (at least after the initial surprise) then

$foo=bar();
(checkForException($foo)) ? exit($foo->errorCode()) : process($foo);

While shorter code doesn't necessarily mean more readable code, I find this to be somewhere in the middle of the two "standard" ways above.

In other words, instead of

if($foo) {
    bar();
}
else {
    // there is no real reason for this to exist, since 
    // I have nothing to write here, but I want to conform 
    // to the general coding practices and my coding OCD
}

One could simply write

$foo && bar();

So what is the reasoning behind this not seeing much use? Can it be as simple as "Don't reinvent the wheel, write the more readable if/else, and if you really want to shorten it, that's what ternary operator is for"?

EDIT: Please keep in mind that the above code was quickly derived from the original code and was mean to be just an example of the use of "short circuit" code. If you can, please restrain from suggesting code improvements, since that was not the desired outcome of the question.

Example No.2

userCheckedTheBox($user) && displayAppropriateInfo();

Answer 1

While $foo && bar(); is fewer lines of code it's much less readable. Making your code easy to understand is usually more important than reducing the total LoC. Even if it's you're not working in an environment with multiple programmers, you will have to come back and read your code at some point in the future, and you probably won't be able to remember what the rationale was behind every line of code (Eagleson's Law).

Generally, you should limit the use of these kinds of statements to only those cases where the programmer's intent is absolutely clear. In my opinion, it's very bad practice to have code which tests a condition and code which actively modifies the current state of the program on the same statement.

Here's one acceptable use for this kind of code:

$isValidUser = $userName && usernameIsValid();

Here, both sides of the && operator are testing a condition, the fact that the right side is calling a function to do that does not harm the readability of the code.

Answer 2

There's an old technique which I believe was popular in hacked-together perl scripts to show errors. pseudo-code:

myFunction( ) || exitWithError( "Uh-oh" )

When coding-to-a-deadline, and when the user interface doesn't need to be stellar, it's a quick way to avoid errors.

The style is also popular in javascript for default parameters:

function myfunction(foo) {
    foo = foo || 0;
    // note that a non-zero default won't work so well,
    // because the user could call the function with 0
}

and for null-checks:

var bar = foo && foo.property;

I find that once you're used to it, it's very readable and often more intuitive than if/else or ?:. But you should only use it when it makes sense. Using it everywhere is going to get very confusing. For example, in your example, you should not use it. Personally I use it for simple error checking and some default values. In big projects, you almost always want to do a lot more when an error occurs, so in those cases you shouldn't use this.

Also you should be careful; this only works in languages which have short-circuit evaluation (http://en.wikipedia.org/wiki/Short-circuit_evaluation). And sometimes and and or are short-circuit, while && and || are not.

myfunction() or die("I'm melting!"); is also quite satisfying to write.

Finally, empty else blocks as a rule is something I've never seen before, or heard anyone recommend. It seems very pointless to me. The most readable option for your example is, quite simply:

if( $foo ) {
    bar( );
}

Answer 3

For errors you should use real exceptions:

try {
  $foo = bar();
} catch(FooException $e) {
  exit($e->errorCode);
}
process($foo);

See the documentation for errorhandling.

Answer 4

What ever that code is doing, returning an instance of CustomException just doesn't add up. Why not change the function definition a little:

function bar()
{
     $stuff = true;
     if ($stuff === true)
     {
         return 'Value on success';
     }
     //something went wrong:
     throw new CustomException('You messed up');
}
try
{//here's the outlandish try-catch block
     $foo = bar();
}
catch (CustomException $e)
{
    exit($e->message());//fugly exit call, work on this, too
}
//carry on here, no exception was thrown

Also, calling that second function (checkForException($foo)) is just absurd. Function calls are cheap, but not free. You want to know if the function returned an instance of CustomException? Don't turn that into a function, but use instanceof. Using short-circuit to keep the number of chars (ad thus parse-time) down, while at the same time wasting resources on on all other levels is about as silly as turning up in a V8 mustang on a hyper-miling course.

Answer 5

Another possible Solution for your problem:

$foo = bar();
!checkForException($foo) or exit($foo->errorCode);
process($foo);

But better change !checkForException for isNoException or something along those lines.