<?php
  function getMd4Pwd($pwd) {
    $pwd = trim($pwd);
    if (strlen($pwd) <= 0)
      return "";
    unset($arrOut);
    $strCmd = "/usr/local/bin/md4sum ".$pwd;
    exec($strCmd,$arrOut);
    return strtoupper($arrOut[0]);
  }
?>

But wait! It gets worse... not only are they not using md5(), they execute a shell script to get an MD4 hash! Really you can't make this stuff up...

And what is /usr/local/bin/md4sum you may ask? Well let me show you...

#!/usr/bin/perl -w
use Digest::MD4;
use Unicode::String qw( utf8 );
Unicode::String->stringify_as( "utf16" );
$u8 = utf8( shift );
print Digest::MD4->hexhash($u8->byteswap), "\n";

So we have a PHP script that calls a Perl script to generate an obsolete, insecure MD4 hash. Not only that but Perl doesn't even have MD4 by default, you have explicitly install it. Um...WTF?!

I wanted to share this. The previous developers didn't use any source control, they simply did the same thing by renaming the old files and added new ones. The file names have been changed to protect the guilty:

This WTF is completely a human problem. Given a bad situation people (especially bad programmers) will find the ways to make it worse. The problem gets worse when dumb developers start using those old libraries! Now your messy directories have turned into software requirements! This may sound retarded but this has happened, and will happen!

Remember source control is to your code as accounting is to your business.

<?php
$query = 'SELECT * FROM sometable';
$result = mysql_query($query,$connection);
$count = mysql_num_rows($result);
?>

It should be obvious what's wrong in the example. To count the number of rows all data is needlessly requested and the rows counted in PHP. As the table grows these three lines will get slower and slower. Most people take data transfer from the DB server for granted. However we always should be as efficient as possible, since little things can quickly multiply into big problems.

The fix is relatively simple:

<?php
$query = 'SELECT COUNT(*) FROM sometable';
$result = mysql_query($query,$connection);
list($count) = mysql_fetch_array($result);
?>

Using count(*) will return just the number of rows. Much more efficient.

I guess nobody told them about the str_pad() function.

function getVanPic($item_id) {
    $pic_path = "/path/to/pic/dir";
    if ($item_id >= 1000000) {
        $strPicName = $item_id;
    }
    else if ($item_id >= 100000) {
        $strPicName = "0".$item_id;
    }
    else if ($item_id >= 10000) {
        $strPicName = "00".$item_id;
    }
    else if ($item_id >= 1000) {
        $strPicName = "000".$item_id;
    }
    else if ($item_id >= 100) {
        $strPicName = "0000".$item_id;
    }
    else if ($item_id >= 10) {
        $strPicName = "00000".$item_id;
    }
    else {
        $strPicName = "000000".$item_id;
    }
    $strPicName1 = "w".$strPicName.".jpg";
    if (file_exists($pic_path."/".$strPicName1)) {
        return "van/".$strPicName1;
    }
    $strPicName1 = "W".$strPicName.".JPG";
    if (file_exists($pic_path."/".$strPicName1)) {
        return "van/".$strPicName1;
    }
    return "";
}

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!

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.