8
\$\begingroup\$

Which of the following way of checking whether a function returns TRUE or FALSE is best, in terms of efficiency and code readability?

I was told by a friend that Method B is a good practice, but I believe Method A is better since it checks whether function returns TRUE then assigns.

Method A: (Checks whether function is TRUE and then assigns.)

if ($result = $db->getResults($id)) {
    echo $result;
} else {
    echo 'fail';
}

Method B: (First assigns the value, then checkstwo operations.)

$result = $db->getResults(90);
if ($result) {
    echo $result;
} else {
    echo 'fail';
}

public function getResults($no) {
    if ($no > 85) {
        $result = 'pass';
        return $result;
    } else {
       return FALSE;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I agree with your friend, method B \$\endgroup\$ – bodi0 Oct 16 '12 at 6:34
  • \$\begingroup\$ @bodi0 and the reason being? \$\endgroup\$ – meWantToLearn Oct 16 '12 at 6:35
  • \$\begingroup\$ The reason being readability. People who devise new, contrived & fabulous ways of doing old & simple things should have their fingers broken. This is one of those great cases. \$\endgroup\$ – TC1 Oct 16 '12 at 10:37
  • 1
    \$\begingroup\$ @TC1 In general I agree with you, but I don't think side-effects in conditional expressions are always bad. For example, a pseudo-generator in a while block: while ($result = getNextResult()) { processResult($result); } – it would be less readable to express this without the assignment in the conditional part of the expression. \$\endgroup\$ – kojiro Oct 16 '12 at 12:21
  • 1
    \$\begingroup\$ @kojiro That's only because there's no decent support for iterable and foreach in PHP, in almost any other language that construction would be foreach(result in getResults()) or something like that. It's an ugly workaround, but, arguably, necessary evil... \$\endgroup\$ – TC1 Oct 16 '12 at 12:26
10
\$\begingroup\$

Actually, both methods assign the value first before comparing its value. Therefore, in terms of efficiency, there's no difference.

For code readability, you could write extra parentheses around the assignment if you choose method A. Method B has a slight advantage when you have to perform debugging before the branch, i.e.:

$res = $db->getResult(95);
var_dump($res); // added for debugging without having to move the code out of the condition
echo $res ? $res : 'fail';

The ternary operator I've used above is yet another way to do your branching; its efficiency is in the same ballpark though. Since the variable itself is used solely in the truthy branch you can also use the shortened ternary operator (>= 5.3) like so:

echo $res ?: 'fail';

Although ternary operators are a powerful concept, they're easy to abuse as well and can cause either bugs or a WTF during code review. My personal guideline is to start favouring an if/else construct when either the condition has multiple sub-expressions or when the manual is opened under operator precedence.

\$\endgroup\$
  • 1
    \$\begingroup\$ Just a minor amendment. Ternary is an amazingly powerful tool, but know when to use it. I say this because most people tend to abuse it when they first learn of it. The above is a good example of proper ternary. However, if they start becoming too long, or too complicated, or god forbid, nested, you'll want to revert to using if/else statements instead. Finally, in some instances if/else and ternary aren't even necessary. Instead of returning a "pseudo" TRUE/FALSE state (pass/fail), just return the boolean. This produces terser syntax that is still legible, for example: return $no > 85; \$\endgroup\$ – mseancole Oct 16 '12 at 14:36
  • \$\begingroup\$ Just to confirm - in case someone doubts - that both versions do an assignment, check the paste here: ideone.com/5BIgu (look at the second opcode). What PHP is doing is performing the assignment, and then doing the boolean check on the result of that assignment (assignment returns the value assigned as a side-effect). \$\endgroup\$ – TML Oct 16 '12 at 20:54
  • \$\begingroup\$ "or god forbid, nested" and they normally come with this comment "//i dare you to change this" >< \$\endgroup\$ – HenchHacker Oct 26 '12 at 23:05
6
\$\begingroup\$

The performance difference is irrelevant; it is really minor (if not zero) and if such minor differences mattered not using PHP at all would be the only proper solution.

Since you only need the variable in the true-branch of the if statement and it's likely to be something other than just a plain true using if(($foo = bar())) (with the additional parentheses to indicate that it's not supposed to be ==) is the cleanest way - but opinions on that will most likely vary.

\$\endgroup\$
3
\$\begingroup\$
if(!$result = $db->getResult($id)){
    echo 'fail';
}else{
    echo $result;
}

Why do you test against false first and true second? Because, if this is in a function, you may want to choose the easy way out. Like this:

if(!$result = $db->getResult($id)){
    echo 'fail';
    // It failed, it's over... so bail here quickly.
    return false;
}
// We succeeded, continue heavy processing of result
// without being contained in a cumbersome block.
echo $result;

There's no right or wrong way. It's always a choice of design. Do you need to return a false in case of failure, try for false first and return or... continue processing in the main block of the function if it succeeded.

Do remember that PHP is very lax regarding stuff like this. It's always easier and more clear to create variables in place right inside the if. Shorter clean code is easier to read code:

if(($result = $db->getResult($id)) === true){
    // true
}else if($result === false){
    // false
}else{
    // not a bool :) handle it
}

Hope I made sense.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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