Skip navigation.
Home
The PHP WTF
Home

I'm smrt'er.

Submitted by phrax on Thu, 2005-01-06 14:38. Bad Architecture

This was sent in by the same anonymous user that submitted the code for I'm smrt. I guess the poor guy is still maintaining what appears to be some nightmare code. At least it gives us something to snicker at as we write our own nightmare code.

The story starts with our anonymous submitter having to dig through the code to figure out why some of the SQL queries were failing. What he found wasn't pretty...

<?php
function sql_val_format($table, $field, $value) {
   if($value == "") {
       return("NULL");
   } elseif((!eregi('^[a-z]*\(.*\)$', $value)) && (!ereg('^0x', $value))) {
       return("'".addslashes($value)."'");
   } else {
       return($value);
   }
}    // sql_val_format()

function sql_insert($table, $values) { // insert into a table
   GLOBAL $SQL;

   sql_flush_cache();
   $what = $val = false;

   while(list($k, $v) = each($values)) {
       if($what) {
           $what .= ",$k";
           $val .= ",".sql_val_format($table, $k, $v);
       } else {
           $what .= "$k";
           $val .= sql_val_format($table, $k, $v);
       }
   }

   ....
}    // sql_insert()
?>
Our anonymous poster says:
Check out that sql_val_format()! It takes two arguments which it doesn't use, then checks the value against a regexp to determine if it should be quoted. To top this all off, this quirky abstraction layer is built /on top/ of PEAR::DB. So DB::quote() / DB::quoteSmart() could have been used instead.

Functions that take unnecessary arguments are some of worst WTFs. Imagine sql_val_format() being used hundreds of times in the code and then you want to change it just to use the $value argument. You'll have to find it everywhere and make the changes and then extensively test to make sure it didn't break anything. Most of us would probably just deal with the unnecessary requirements and keep using three arguments. This is one of those WTF's that live forever, eternally producing more and more bad code. The best and often the least feasible (money, time, sanity) is to find it everywhere in the code and fix it.

I'm currently re-reading The Pragmatic Programmer: From Journeyman to Master which has a great example of this. Anybody that has ever read it likely remembers the Broken Windows Theory. The Broken Window Theory basically says a single broken window creates the perception of abandonment and soon results in everything going to hell. The same applies to code. You create a broken window like sql_val_format() and it will cause everybody to start thinking the project is already a waste and they in turn will produce crappy code.

Comment viewing options

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

Wow... sql_insert is FRIGHT

Submitted by GrumpySimon (not verified) on Fri, 2005-01-07 01:04.

Wow... sql_insert is FRIGHTENING.

1) Globals are evil, but if you're going to declare it, AT LEAST USE IT.

2) What the hell is $what = $val = false; for? Let's see $val isn't declared, and shouldn't one of those equals be doubled?

My head hurts.

» login or register to post comments

> if you're going to declare

Submitted by Paul (not verified) on Fri, 2005-01-07 01:24.
> if you're going to declare it, AT LEAST USE IT.

He could be using it further down that function though - note the ellipsis

> and shouldn't one of those equals be doubled?

Um, no. But declaring $what and $val as Booleans when they are in fact strings shows there's a pretty shaky hand on the wheel.
» login or register to post comments

But Simon has a point in sa

Submitted by Jonathan (not verified) on Fri, 2005-01-07 02:41.
But Simon has a point in saying that. Maybe it's part of the nightmare plan, but why the hell would you check a variable, if you've manually set it to false? That's like saying you wanna clean up your entire house February 30rd - it ain't gonna happend!
» login or register to post comments

"The Broken Window T

Submitted by foxyshadis1 (not verified) on Sun, 2005-01-09 22:31.

"The Broken Window Theory basically says a single broken window creates the perception of abandonment and soon results in everything going to hell."

I'm reading a Michael Connelly book that states the theory, and I wondered why it went over it twice, and rereading this entry I just realized I read it here first a few days before, not the book. Oops. :p

It looks like they were trying to do something with the two paths, and were going to set $what and $val to something, but just left it half-dead instead. This is overall one of those you shake your head at and look for other work.

Simon, you should get a quick refresher on esotoric C/PHP constructs if you hope to be able to maintain them later. The PHP manual has a decent one.

» login or register to post comments

I'm the submitter, and I'm ju

Submitted by Anonymous (not verified) on Mon, 2005-01-10 17:46.
I'm the submitter, and I'm just following up on some of the comments here. I'm not the original author, I just have to maintain this mess as best I can until it dies the terrible death it deserves.
  1. GLOBAL $SQL - This is defined elsewhere (an init function) and used throughout the code, but I omitted the portions of sql_insert() that use it for brevity. Ugly, yes, but not completely stupid. The GLOBAL $SQL statement isn't a declaration, btw.
  2. $what = $val = false; - This just inits $what & $val to false and is equivalent to $what = false; $val = false; Yes, it's dumb that they're initialized to boolean when used as strings. I suspect it was done that way so the if($what) test fails the first time. Of course, PEAR::DB supports paramater replacement (add '?'s in your SQL statement, pass values in an array, it automagically quotes them and everything - rendering all this code not just retarded and unmaintainable, but redundant as well.)
  3. I forgot to mention that the actual problem was that sql_val_format()'s lame regexp check allowed unquoted values through, so it's ineffective and insecure at the same time.
» login or register to post comments