Quick coding WTF without failure



  • Here's something a co-worker recently stumbled upon:

    <font face="courier new,courier">if (!count($items)>0) { something(); }</font>

    Now, for those that don't know PHP, let me explain what happens: First of all, the count() function is evaluated and that returns an integer. PHP does automatic type conversion and the negation is next in line, so that integer value is converted to a boolean and negated. The final evaluation is the comparison, which is between a boolean and an integer (0) but, because the boolean is the first operand, the integer is translated into it's boolean equivalent, namely false. Finally, the following is evaluated:

    <font face="courier new,courier">if (has_zero_items > false) then { do something }</font>

    where has_zero_items is a boolean and true > false in PHP logic. So, if the array is empty, the condition is satisfied and something() is executed; if the array contains some elements, the function isn't executed. Coincidentally, that's also what the previous developer intended to do, probably thinking along the lines of "if the count is not greater than zero, then do something" without considering operator precedence. He probably intended to write the following, but missed a pair of parenthesis:

    <font face="courier new,courier">if ( !(count($items)>0) ) { something(); }</font>

    The way I find easiest and clearest to express this is

    <font face="courier new,courier">if (!count($items)) { something(); }</font>

    While some friends said they prefer a clear evaluation, without "abusing" typecasting:

    <font face="courier new,courier">if (count($items) == 0) { something(); }</font>

    In Romania, we have a saying which I don't think can be translated to English, but it's interpretation would be "it doesn't matter how stupid you are, as long as you're lucky..."



  • @rohypnol said:

    In Romania, we have a saying which I don't think can be translated to English, but it's interpretation would be "it doesn't matter how stupid you are, as long as you're lucky..."
     

    Fate protects fools, little children, and ships named Enterprise?  :D



  • @shepd said:

    @rohypnol said:

    In Romania, we have a saying which I don't think can be translated to English, but it's interpretation would be "it doesn't matter how stupid you are, as long as you're lucky..."
     

    Fate protects fools, little children, and ships named Enterprise?  :D

     

    +1

    I think the analagous English phrase is something like, "It's better to be lucky than good."



  • @rohypnol said:

    if (!count($items)>0) { something(); }

    Sadly, I've seen the same exact bug many times in production PHP. 



  •  On a related note, I recently found this during a code review:

    CPPUNIT_ASSERT(0 <= percentage <= 1);
    


  • @sootzoo said:

    @shepd said:

    @rohypnol said:

    In Romania, we have a saying which I don't think can be translated to English, but it's interpretation would be "it doesn't matter how stupid you are, as long as you're lucky..."
     

    Fate protects fools, little children, and ships named Enterprise?  :D

     

    +1

    I think the analagous English phrase is something like, "It's better to be lucky than good."

    Stephen Schwartz says it well in "War is a Science", in the musical "Pippin":  "It's smarter to be lucky than it's lucky to be smart."



  • @rohypnol said:

    The way I find easiest and clearest to express this is

    <FONT face="courier new,courier">if (!count($items)) { something(); }</FONT>

    While some friends said they prefer a clear evaluation, without "abusing" typecasting:

    <FONT face="courier new,courier">if (count($items) == 0) { something(); }</FONT>


    What about
    if (empty($items)) { something(); } 
    ? I think that looks the best :)



  •  And you are right, it "looks" best, but it's not also the best choice. The empty() language construct also returns true if the variable is unset or if the variable is a different type, such as aboolean with the value true. As the good developer that you are, you want to catch those errors and treat them, instead of silently ignoring them. I've seen plenty of cases where things falied becausesomeone used empty() instead of !isset() or !count(). Do exactly what you have to do - check if the array $items is empty, NOT if the variable $items is empty by the language's standards.

    This could also lead to potential security holes in applications:

    <font face="courier new,courier">$tmp = $_GET['param'];</font>

    <font face="courier new,courier">if (empty($tmp)) {</font>

    <font face="courier new,courier">   do_some_stuff();</font>

    <font face="courier new,courier">   foreach (file('bash_commands.txt') as $line) $tmp .= $line;</font>

    <font face="courier new,courier">   exec($tmp); </font>

    <font face="courier new,courier">}</font>

    Yeah, you'd never do that, right? Now remember, you're using empty() to check if a variable is empty, instead of checking it's contents... Also, imagine if the foreach() line is added by a n00b that doesn't know exactly what the empty() construct does, but he assumes something based on the context.



  • @rohypnol said:

    Yeah, you'd never do that, right?
     

    You're right, I would never do that. But that's to do with the fact that $tmp came form an external source, rather than issues with empty(), which are arguably a problem with the language (specifically, PHP), rather than general programming practices.

    Although you might argue it's about  good programming practices for PHP. :)



  • @dhromed said:

    @rohypnol said:

    Yeah, you'd never do that, right?
     

    You're right, I would never do that. But that's to do with the fact that $tmp came form an external source, rather than issues with empty(), which are arguably a problem with the language (specifically, PHP), rather than general programming practices.

    Although you might argue it's about  good programming practices for PHP. :)

    Definitely not.  Honestly, I almost never use the empty() construct simply because it's not all that useful in PHP. 



  • @rohypnol said:

    <FONT face="Courier New">if (!count($items)>0) { something(); }</FONT>

    Were this left alone, some automated process would eventually have converted it to:

    <FONT face="Courier New">if (count($items) <= 0) { something(); }</FONT> 

    ...and people would have scratched their heads for hours trying to figure out WTF the original coder was thinking.


Log in to reply