Skip navigation.
Home
The PHP WTF
Home

The Launch of the PHP WTF

Submitted by phrax on Mon, 2004-08-09 14:45. Wonky Code

Welcome to The PHP WTF. The idea came from Alex of The Daily WTF. Alex has many examples of VB/Java nightmares and I figured the World needs examples of the bad PHP that people write.

To start things off here's an example from my personal collection. Originally published on my personal blog at http://benzo.tummytoons.com/node/view/66, it demonstrates one of the worse things in PHP, variable variables! Ugh!

I have found that explaining bad code to non-programmers is usually followed up by, "isn't it a matter of style?". Programming styles may differ but there is a difference between bad style and bad code.

This is today's example of bad code. 
case 2:
  $stSQL = "UPDATE associate set ";
  for ($i = 0; $i < sizeof($arrReferralName); $i++) {
    if (strlen(trim(${"assc_".$arrReferralName[$i]."code"})) > 0)
      $stSQL = $stSQL." assc_".$arrReferralName[$i].
               "code=".${"assc_".$arrReferralName[$i]."code"}.",";
    else
      $stSQL = $stSQL." assc_".$arrReferralName[$i]."code=NULL,";
    $stSQL = $stSQL." assc_".$arrReferralName[$i]."='".${"assc_".$arrReferralName[$i]}."'";
    if ($i < (sizeof($arrReferralName) - 1))
      $stSQL = $stSQL.",";     
  }
  $stSQL = $stSQL." WHERE assc_id=".$ul["assc_id"];
  mysql_db_query($mydatabase,$stSQL);
break;
Other than generating a SQL query, nobody, including the original author can tell what the above code is supposed to do at a glance. The code's purpose has been hidden behind variable variables, a PHP 'feature' that should rarely be used. A requirement of good code is its purpose is immediately apparent. The example requires a lot of investigation to figure out what it does. Investigation takes a lot of effort, it is frustrating, it is aggrivating and it is a waste of time (and thus money). This is what I rewrote it to. The functionality has been updated to include better data checking and the structure changed to better show the code's purpose. 
case 2:
  // Update the Phone Numbers
  $stSQL = "UPDATE associate set ";
  foreach ($arrReferralName as $refname) { // $arrReferralName from inc_referral.php (in common/)
    // DB Column Names
    $areacode = 'assc_'.$refname.'code';
    $phonenum = 'assc_'.$refname;
    // DB Column Data
    $codeData  = format_phonecode($_POST[$areacode]); 
    $phoneData = format_phonenum($_POST[$phonenum]);
    if ($codeData == '')
      $codeData = 'NULL'; 
    // update the area code
    $stSQL .= "$areacode=$codeData, ";
    // update the phone num
    $stSQL .= "$phonenum='$phoneData' ,";
  }
  // chop the trailing comma
  $stSQL = substr($stSQL,0,-1);
  $stSQL .=" WHERE assc_id=".$ul["assc_id"];
  mysql_db_query($mydatabase,$stSQL);
break;

The new code is double the length of the original, but it is much simplier. It is now apparent that the code uses POST data to generate a SQL query for updating phone number data. The new code uses the $_POST super-variable, and references the data as elements of the array. $_POST[$varname] is a lot easier to understand than ${"assc_".$arrReferralName[$i]."code"}. Especially since the code is old and written to use automatically registered globals!

Being clever is not always a good thing. There should be a balance between clever code and good code. Good code is considerate to those that come after you. Clever code, like above, leaves a legacy of frustration, resentment and expense in time and money.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.

Submitted by Alex Papadimoulis (not verified) on Mon, 2004-09-20 04:09.
Lookin Good! I mean, literally, you've got a slick look and everything ;-) I gotta go outside of the standard skin one of these days!

/envious
» reply

Submitted by Imablog (not verified) on Mon, 2004-09-20 08:55.
I don't really code like that, do I?
Hopefully the freshly launched PHP WTF site gets lots of contributions so that people like me can see what kinds of dumb programming and style mistakes not to make....
» reply

Submitted by Roland Tanglao (not verified) on Mon, 2004-09-20 11:35.
hey what about drupal?
» reply

Submitted by Phil Scott (not verified) on Tue, 2004-10-12 11:02.
This site looks great! And another site to add to my RSS reader.
» reply

Submitted by Eli Lilly (not verified) on Wed, 2004-10-13 07:38.
For me, a WTF would be seeing the mysql-specific database calls sprinkled all over the place. Wrap that stuff!
» reply

Submitted by BDKR (not verified) on Fri, 2004-10-15 09:51.
>>Being clever is not always a good thing. There should be a balance between clever code and good code. Good code is considerate to those that come after you. Clever code, like above, leaves a legacy of frustration, resentment and expense in time and money.
» reply

Submitted by BDKR (not verified) on Fri, 2004-10-15 09:52.
>>Being clever is not always a good thing. There should be a balance between clever code
>> and good code. Good code is considerate to those that come after you. Clever code,
>> like above, leaves a legacy of frustration, resentment and expense in time and money.

Well, since the last post didn't work, I'll try again. Good show, this is right on the money and so correct it hurts!
» reply

Submitted by BDKR (not verified) on Fri, 2004-10-15 10:32.
Err...., you missed one or two.

1) Why count the size of the array at each iteration?
2) Why use double quotes for strings if there is no need to evaluate
the string?

Cheers,
BDKR
» reply

Submitted by Mr.SQL (not verified) on Sat, 2004-10-16 02:35.
I hope you simplified this aren�t you?
Because it�s open to SQL Injection!
» reply

Submitted by Aderyn (not verified) on Sat, 2004-10-16 04:45.
Oh please. mr.sql, that was propably the stupidest thing I've heard today (_After_ reading through all the WTF:s on this site).

To the author of this WTF: You do know that mysql_db_query() has been deprecated since PHP 3 or something?
» reply

Submitted by Anonymous on Sat, 2004-10-16 09:03.
To: Aderyn,

Haha. Yea I know, I've never seen it used except on this site, even in my PHP3 days. I didn't even know the function existed until after I saw the code.

I figure that if you're working with somebody else's bad code you should still try to remain consistant with their design and philosophy. The reason is that you don't know what might break if you make medium or big changes. Say instead of mysql_db_query() I had added in mysql_select_db() and then used mysql_query(). By changing the way a DB is selected in the application there is a chance that it might break something elsewhere. In these cases, to be safe, it is better to remain consistant.
» reply

Submitted by foxyshadis (not verified) on Sun, 2004-10-17 03:48.
BDKR:

Double vs. single strings in PHP are a constant source of wtfs when C programmers come to PHP. A database update engine a friend built ran in half the time (and stopped timing out) after I told her to use singles. A better idea would have been to keep both normal, and use backtick (another common wtf) or some other non-traditional character.

"Because perl did it" is not a great excuse, especially since most perlers eshew php and prefer python if anything else, but it's too late now.
» reply

Submitted by chris (not verified) on Tue, 2004-10-19 18:38.
You do realize that cating strings is slower than manipulating arrays, right?

// meh...
/* -->8-- */
// update the area code
$stSQL .= "$areacode=$codeData, ";

// update the phone num
$stSQL .= "$phonenum='$phoneData' ,";
/* -->8-- */

// yay!
/* -->8-- */
// update the area code
$update_array[] = "$areacode=$codeData";

// update the phone num
$update_array[] = "$phonenum='$phoneData'";

$query = "UPDATE tablename SET " . implode(', ', $update_array) ....;
/* -->8-- */

And of course, you should always be checking to see if a variable actually exists, rather than simply assuming it will be there (*cough*$_POST[$areacode]*cough*). I hope this is just an oversight while making an example of poor code.
» reply

Submitted by Anonymous on Tue, 2004-10-19 21:37.

Chris: you do realize you are WRONG! Perhaps you should test it code before you speak up...

Here's a shell script for you: 
#!/usr/bin/php
<?php
// test if concatenating is faster / slower than manipulating arrays
$data = '12345'; 
$iterations = 100000; 
// Time String Concatenation
$time_start = microtime_float(); 
$bigString = ''; 
for ($i = 0; $i < $iterations; $i++) 
{
   $bigString .= "var='$data',"; 
}
$bigString = substr($bigString,0,-1);
echo strlen($bigString)." - bigString, Time: ";
echo microtime_float()-$time_start ."\n"; 
// Time Array Concatenation
$time_start2 = microtime_float(); 
$x = array(); 
for ($i = 0; $i < $iterations; $i++)
{
    $x[] = "var='$data'"; 
}
$bigString2 = implode(',',$x); 
echo strlen($bigString2)." - bigString2, Time: ";
echo microtime_float()-$time_start2 ."\n"; 
function microtime_float()
{
   list($usec, $sec) = explode(" ", microtime());
   return ((float)$usec + (float)$sec);
} 
?>

Now run this in a shell: for i in 1 2 3 4 5; do echo "Run Number #$i"; ./test_speed.php; echo; done

You should get something similar to this: 

Run Number #1
1199999 - bigString, Time: 0.5843870639801
1199999 - bigString2, Time: 1.0092749595642
Run Number #2
1199999 - bigString, Time: 0.34683203697205
1199999 - bigString2, Time: 1.0810549259186
Run Number #3
1199999 - bigString, Time: 0.35090112686157
1199999 - bigString2, Time: 1.0080449581146
Run Number #4
1199999 - bigString, Time: 0.3545548915863
1199999 - bigString2, Time: 1.0761342048645
Run Number #5
1199999 - bigString, Time: 0.35537505149841
1199999 - bigString2, Time: 1.0508508682251

Not only is using an array slower, it also users more RAM. Yea, great tip... perhaps you should submit some of your code, I'm sure it'll make a good WTF for the site!

---

Also I know that $_POST[$areacode] exists! Unless somebody changes the form and doesn't bother updating the code, or vice versa, that code will always be valid.

Perhaps next time you should check your facts!

» reply

Submitted by foxyshadis (not verified) on Thu, 2004-10-21 07:34.
Actually, come to think of it, it is vulnerable to sql injection. What if $refname is specially formulated to exploit the system? What if format_phone*() returns an invalid, exploitable string? And god knows what the compiler will do. I'm astonished no one caught this before now, why, programmers could have been actively exploiting their own SQL for decades! We obviously need to remove their sql writing capabilities NOW.

Hey phrax, man, chill. No need to slam and gloat; just gently correct and let your results speak for themselves. It'll keep people coming back. ^_~
» reply

Submitted by Tom Robinson (not verified) on Mon, 2004-10-25 17:35.
This code should be faster than repeated ".=" or array implode: $query = 'UPDATE tablename SET ' . "$areacode=$codeData, " . // update the area code "$phonenum='$phoneData'" . // update the phone num "WHERE ..."; But if those line-by-line comments aren't essential, this is even faster. And more readable: doSQL(" UPDATE tablename SET $areacode = $codeData, $phonenum = '$phoneData' WHERE ... "); I go for readability most of all, because it helps avoid that unmeasurable performance penalty: code that doesn't perform as intended
» reply

Submitted by Tom Robinson (not verified) on Mon, 2004-10-25 17:37.
Let's try that again with <BR> since this system stripped the <PRE> tags:

This code should be faster than repeated ".=" or array implode:


$query = 'UPDATE tablename SET ' .
"$areacode=$codeData, " . // update the area code
"$phonenum='$phoneData'" . // update the phone num
"WHERE ...";


But if those line-by-line comments aren't essential, this is even faster. And more readable:


doSQL("
UPDATE tablename
SET $areacode = $codeData,
$phonenum = '$phoneData'
WHERE ... ");



I go for readability most of all, because it helps avoid that unmeasurable performance penalty: code that doesn't perform as intended
» reply

Post new comment




*

  • Web and e-mail addresses are automatically converted into links.
  • Allowed HTML tags: <a> <em> <strong> <cite> <code> <ul> <ol> <li> <dl> <dt> <dd> <pre> <p> <br /> <br>
More information about formatting options