Published 2005-08-25 08:47:02

Fixing someone else's code is frequently the most annoying job that anyone with some intellegence has to do. It's one of those, pay the rent, scream and run around jobs..

I'm sure other languages suffer as well, but thanks to PHP's learning curve, software written in the language often falls into 3 categories
  • Half a clue (HAC)
  • Reasonably worked out
  • Over engineered
This week, unfortunatly, I had to attack both the HAC and the Over engineered, (not on the same project thankfully).

The problem with the HAC project was the end users had found a few minor bugs. (this was a PHP application that had been paid for by a client - kind of like a shareware type thing.). But luckly (or perhaps less luckly for me) they got the source code.

Bugs are a fact of life for all software, whether you find them when developing, or as an end user. There's bugs in all my software or features depending on how you look at it (see the bug list on pear.). But the difference between average and good programmers (IMO) is the ability to write code that someone else has a chance of understanding.  (and sending in bug fixes...hint hint....)

Untraceable code paths.
The first gripe I had with the HAC was the horrific use of random functions included from a wealth of files, making following the codepath next to impossible. (which file will this function come from? = grep again...)

The bug I had to solve was supposed to be quite simple the host name for an img src= tag, was pointing at the wrong server. (the application lives on a private development server running behind a reverse proxy, and hence the wrong host name)

Same code repeated in global scope
Like alot of PHP applications, this one had one script to respond to each url request (rather than a bootstrapper and class loader). The script then did the usual crap of loading config, libraries (bad joke there) etc. It then went down and had a big if block for each action.. (all in global scope). Apart from making the file rather large, it also would have made managing global changes to the site more complex. This is where this concept makes a world of difference.

class page_action extends applicaton_base { } 


Mixing PHP and HTML badly
Unfortunatly, no-where within that code was it immediatly clear where the image url or HTML was comming from. Yes another of those deadly sins, splattering HTML all over the PHP code,  resulting in some very large php scripts, which you have to look really hard to determine the what on earth is going on due to the excessive noise from the HTML..

HTML + Databases + $variables +  eval = trouble waiting to happen
When I finally tracked it down, (after grepping for a few functions that seemed possible candidates) I found that the tag was contained in a small snippet of HTML stored in the database. The HTML in the database went something like src="$site_base_url/$the_image_url". The author had decided that the wonderfully secure eval() function made a great dynamic layout tool.

Every variable should start somewhere
Now I knew what the variable was. The next task was to work out where it was comming from. A grep of the source indicated lots of instances of it being used, but not a single one creating it.

This is my pet gripe with $smarty->assignVal(), (and quite a few other template engines) you have another instance of data getting used with a great disconnect with the source of the original data.  Making backtracking code and locating where something occurred considerably more difficult. Let alone making security auditing next to impossible.

Global is almost as evil as eval()
Almost all the functions in the applications had a list of 20 or more global variables. This instantly made all the function completely un-re-usable. (it hard coded the implementation to the application). and goes back to the problem of tracking down where variables where created.

How may evil functions does PHP have - extract()
In the end, the culprit of this design was the wonderful function extract(), It had been used to convert the return array from a database query, and filled those wonderfull global variables. By making the result set array available globally the could would have been so much more readable, and it would only involve a small amount of extra typing (Copy and paste anyone). So after all that digging, It turned out you had to use the application interface to change that variable......

I guess in the end, this all goes back to the Microsoft philosopy, if we all made perfect software, then there would be no work for everyone fixing it. Although some of us would like to doing more with our lives ;)

Mentioned By:
www.sitepoint.com : SitePoint Blogs » Blog Archive » How Readable is Your PHP? (996 referals)
www.php.net.my : PHP.net.my - Kumpulan Pengguna PHP Malaysia (217 referals)
google.com : april (138 referals)
phpgg.nl : phpGG.nl | Nederlandse PHP gebruikersgroep (134 referals)
www.phpmag.net :  International PHP Magazine - Cutting-Edge Technologies for Web Professionals (130 referals)
planet-php.org : Planet PHP (118 referals)
google.com : december (115 referals)
www.reiersol.com : PHP In Action (113 referals)
www.reiersol.com : PHP In Action | Another (seventh?) deadly PHP sin (111 referals)
phparch.com : php | architect - The PHP Magazine for PHP Professionals (107 referals)
google.com : mixing php and html (103 referals)
www.sitepoint.com : SitePoint Blogs (95 referals)
blog.syntaxcms.org : Exellent code writing tips - Syntax Framework (94 referals)
blog.syntaxcms.org : Syntax Framework (88 referals)
www.nexen.net : Nexen.net : Portail francais PHP et MySQL (77 referals)
www.phpdeveloper.org : PHPDeveloper.org: PHP News, Views, and Community (71 referals)
phpgg.nl : Hoe moet het NIET? | phpGG.nl (41 referals)
google.com : php week (38 referals)
blog.agilephp.com : Another (seventh?) deadly PHP sin – PHP in Action (37 referals)
google.com : mixing php html (33 referals)

Comments

Facetious Observation
So I guess what you are trying to say is that the programmer's coding ability is about on par with your spelling/proof-reading ability?

Just busting your chops, of course ;)

BTW- in your Error messages for this form, empty is spelled "empty", not "emtpy
(unless that is what you meant- I'll have to look up the definition of "emtpy" to be sure)
#0 - C Drozdowski ( Link) on 2005-08-25 09:30:15 Delete Comment
published a bit early
should be proof read better now ;) - published a bit early there.

Errors should now be fixed - thanks
#1 - Alan Knowles ( Link) on 2005-08-25 10:05:03 Delete Comment
An applicaton immediatly fixed
Well, you probably missed a few typos here and there ;)

looking forward for your post about over engineered code.

#2 - xavier ( Link) on 2005-08-25 18:34:11 Delete Comment
Wonderful
Thanks for the post, I love getting in to the minds of other developers. Having to grep a directory structure to find out where the _hell_ a variable is comming from is hot stuff, btw.
#3 - Luke K ( Link) on 2005-08-25 23:45:49 Delete Comment
Super Duper
Reminds me of some code audits which I had done previously. Certain users don't even document their code in the slightest and you have to figure out how to fix or modify applications.

Others for example allow their applications to write arbitrary data into their webroot which does not help in the slightest.

You have got to just love newbies! ;)
#4 - Jacques Marneweck ( Link) on 2005-08-26 00:01:53 Delete Comment
Well, ok, but ...
(sry for mistakes, im frensh)
One of the problems with PHP is that it is easy to start using it without any programming notion before.
So, there are a lot of bad coded softwares.
But, where you all perfect when you started? Whe cant all have a course payed to learn proper programming.
The importent part is to explain to all, like in this article, what is dangerous, and even thoes who use php since 10 years learn somthing.
#5 - Sky ( Link) on 2005-08-26 15:49:46 Delete Comment
Even eval is abused in Perl
What's scary is that even the coders of the Advanced Web Statistics (awstats) have misused eval and people have been able to remotely execute commands on the said servers.

It may be better if we were to place a huge warning caption to the eval function page to advise users to avoid using eval?
#6 - Jacques Marneweck ( Link) on 2005-08-26 23:17:09 Delete Comment
What are you using for a dev environment?
I've been using Dreamweaver lately, just the code view, and it has very useful search functions that allow me to search files, directories and entire sites at once. I know other apps have this feature too.
#7 - Atroxodisse ( Link) on 2005-08-27 07:29:46 Delete Comment
In general
Though I understand the writers frustration I think extract() is a great tool to write flexible functions, and once you know it is used (and the function names make sense) it ain't that hard to debug.

Atroxodisse: in UltraEdit you get a real nice overview of all search hits in multiple files. A lot cheaper and easier for coding than Dreamweaver if you ask me.
#8 - Chris ( Link) on 2005-08-28 17:14:00 Delete Comment
you serious?
"I've been using Dreamweaver lately, just the code view, and it has very useful search functions that allow me to search files, directories and entire sites at once."

are you trying to compare Dreamweavers [i]search[/i] capabilties with grep?

obviuosly another windows user.
#9 - thorpe ( Link) on 2005-11-10 10:44:54 Delete Comment
Cheers
Thanks for sharing this. Made a post about it on my site.

-Tim
http://php-coding-practices.com
#10 - Tim ( Link) on 2007-05-04 19:49:30 Delete Comment
Newby
I read your post and am learning. funny you should mention a job worth running around pulling your hair out. I'm a designer, yet more and more the job calls for programming code ugh!. I had to rework a php form that a previous designer had copied and hacked to bits. I was following that damn string all over the place, into dead ends weird folders, etc etc., boss saying... why aren't these changes done, you know. Anyway, do you know of a place online that gives step by step coding instructions for php?? would help tremendously to know the basic rules. I come from basic html/css.
#11 - Newby ( Link) on 2008-11-19 18:58:22 Delete Comment

Add Your Comment

Follow us on