Home > PHP/MySQL > 17 PHP Practices That Should Be Banished Forever

17 PHP Practices That Should Be Banished Forever

by Sam on August 1, 2009 · 63 comments

in PHP/MySQL

Stop
The following is a list of 17 PHP programming practices that in my opinion should be banished into oblivion. Forever and ever. Amen. Please note that this list is in no particular order. So, without further ado:



1. Using Register Globals

Most PHP programmers will have heard of this at least once. ‘Don’t rely on register globals!’. This issue has been around for quite some time. The disturbing thing is that still, in the year 2009, I find that many programmers still rely on this now deprecated feature of PHP. Using register globals is a bad idea. Stop relying on it. As a matter of fact turn it in off your php.ini configuration file. As of PHP 4.2.0 (if you’re using this or an older version you really should consider upgrading to a newer version, come on its free!) the default setting went from on to off. You absolutely do NOT need rogue variables swimming around in your application.

2. Relying on Magic Quotes

One of the inherent downsides to PHP is its ease of use. In attempting to make things as easy as possible for developers, the creators of the language have, with good intentions, put in place some features that are in direct conflict with good programming practice. When enabled, magic quotes ‘automagically’ escapes all user input, that is, renders it safe to be placed in a database. This creates a potential security hole in your application because you need to always know if its on or off. If its off and you don’t manually escape the user input you leave yourself susceptible to SQL injection attacks. If its on and you do manually escape user input then you will end up with double escaped data filled with lots of annoying backslashes. The best thing to do is to turn it off if you can and escape and unescape data manually using the addslashes() and stripslashes() functions respectively.

3. Not Sanitizing and Validating User Input

This is not limited to PHP, it is a general programming mantra independent of chosen language or platform; never ever trust user input. It is alarming, the number of unseasoned developers who accept user input via an html form or request data and simply throw this data right into their application via variables or a database without employing any form of input validation or sanitization. This is extremely dangerous and in many cases comes right back to laziness. Barring those who really just don’t know any better, there are some developers out there that are aware of all the risks but are simply just too lazy to write an extra few lines of code to ensure that the data submitted by users is what it should be. It is certainly not enough to rely on client side scripting as client side scripts can easily be circumvented. Take the time to properly sanitize and validate all user input. Its more than worth it.

4. Mixing HTML, PHP, SQL, JS, CSS, Etc, Etc

This just looks ugly and makes for a giant pot of spaghetti. Avoid doing it at all costs. If you must (though it defeats me as to why anyone ever ‘must’), at least separate each language into a separate section of the file as best as possible.

5. Using Short/ASP Style Tags

This is bad for a zillion reasons. I’ll give you two, one for each style. Short tags (``) cause conflicts with xml parsers because the xml language uses these tags as well. The concept of ASP style tags (`<% //code %>`) is an utterly stupid one simply because PHP is not ASP. Don’t use them. No excuses, just don’t use them.

6. Not Documenting Code

Any code that took thought to write will also take thought to understand. Undocumented or poorly documented code presents a problem for you when you have to review code you wrote in the past. You will have to wade through all of the functions and class methods etc. to find out what parameters they take, what they return and quite literally, what they do. If your code is procedural then than can even be a worse nightmare. If you for any reason need to have someone else modify your code I guarantee it will be a nightmare. I have had the displeasure of working with code that had absolutely no meaningful documentation. Tasks that would have taken me a few hours to accomplish had the code been properly commented took me days to finish. I have since vowed to NEVER take on any job involving poorly documented code. I would much rather write the entire thing from scratch.

7. Using User Input as an Argument in eval()

The function eval() is evil. eval() takes a string as an argument and allows that string to be executed as PHP code. That idea almost always appeals to newbie developers (as I must admit it did to me). This is no doubt a very powerful tool but it can be monumentously dangerous. Here’s a quote regarding its use from Rasmus Lerdorf, the creator of PHP:

If eval() is the answer, you’re almost certainly asking the wrong question.

I don’t absolutely agree. Personally I don’t use eval() at all but I have seen it used in some very creative ways. In every single one of those cases I have found an alternative that would give the same result. I just do not fancy the idea of dynamic code evaluation. If you must use the eval() function, never ever user user input as an argument for the function. You’d just be setting yourself up to be on the receiving end of a code injection exploit.

8. Recursive Inclusion

The days of having a laundry list of includes at the beginning of each and every file in your application are long past. If you are still running a version of PHP earlier than 5, upgrade and make use of the __autoload() magic function or the autoloading features of the Standard PHP Library (SPL).

9. Unnecessarily Wrapping Native Functions in Classes

Some programmers nowadays have adopted a ridiculous habit of wrapping almost every native PHP function they need to use in their application in a bloated class. I find this to be really annoying. This obsession with object oriented programming is all well and good, but procedural programming has its place. It has been my experience that writing tons of unnecessary classes actually slows down your application. Noticeably. Guys, come on! Stop writing wrappers for cURL!

10. Using die() to ‘Handle’ Errors

This one really grinds my gears because it is a result of sheer laziness. The die() function is a crude way to acknowledge errors and really should not even be considered a method of error handling because the error is not ‘handled’ at all it is simply acknowleged. Try at least using trigger_error() in conjunction with set_error_handler() if you’re not ready to make the step up to using exceptions but die() really needs to just, well, die.

11. Using Regex Functions for Basic String Manipulation.

This one is pretty straightforward. Yes, using regex is cool. Yes, using regex MIGHT make you seem like some kind of regex ninja. No you need not use them every single chance you get. Regular expressions are a very powerful tool which when used correctly, can accomplish some pretty amazing things as demonstrated in this excellent article. However, they are far slower than the native PHP string manipulation functions and tend to make your code more difficult to understand. So, if at all possible, try to substitute the ereg_* and preg_* family of functions with the str_* and ctype* family of string manipulation functions.

12. Not Closing Database Connections

We all want to open database connections but no-one wants to close them. If you have made a connection to a database then kindly close it when you’re done. Having it open is an additional overhead. Also avoid opening more than one connection to the same database in your scripts. Its better to reuse a connection than to keep opening and closing connections from a server resource perspective. To accomplish this, consider using a singleton approach.

13. Error Suppression Using The ‘@’ Symbol

Suppressing errors in general is not wise. Errors should be logged and handled gracefully. Using the error suppression operator in your code only makes debugging more difficult. Its very frustrating when your code does not do what it ought to do and you don’t receive any errors only to find hours later that had you not used the error suppression operator on some function x, you would have known that function x was causing an error. Bad practice. Stop it now.

14. Inappropriate Display of Errors

One of the signs that a website is being run by a complete amateur is seeing something like this after loading a page:

Fatal error: Call to undefined function: mysql_connect() in /home/httpd/mysite/php/myapp/include/db.inc on line 166

Users should never ever see errors like this. Once in a production environment, errors like these should be logged and the user sent a more user friendly message which divulges no details about the inner workings of your application. Error messages like the one above reveal intimate details of your code that not everyone should be privy to such as your directory structure. Conversely, in a development environment all errors should be displayed so you as the developer can quickly easily see where your code has gone wrong. Use the display_errors php.ini directive to control whether or not errors are displayed in your application.

15. Unquoted Array Keys

I very often come across code looking like this:

$myArray[boy] = 'harry';

This is bad coding practice. Array keys should always be enclosed in single quotes unless the key itself is a variable. If they aren’t, the PHP interpreter will consider the key a constant and search for a constant definition corresponding to that key. After not finding a constant definition, only then will the key be converted to a string. That just gives the interpreter more work to do. The above example written correctly would then become:

$myArray['boy'] = 'harry';

16. Using Uninitialized Variables

Due to the laid back nature of the PHP interpreter, it is possible to use a variable that has not been initialized as PHP will simply create the variable for you. Not only is this a result of laziness yet again, it makes for very confusing code. Always initialize your variables.

17. Placing Functions in Loop Declaration

Consider the following code:

for ($i = 0; $i < count($myArray); $i++)
{
//Code
}

This code will work, but what happens is that the function count() gets executed on each iteration of the loop. This in turn slows down the entire process. Try doing something like this instead:

$total = count($myArray);
for ($i = 0; $i < $total; $i++)
{
//Code
}



PHP is a very forgiving language and as a result it is easy to develop bad habits. As developers we should always strive to ensure that we are doing this the right way. Doing them the wrong way will always have some undesirable consequence.

Short URL for this post:

http://bit.ly/R7aFH

Comments Map

Location data courtesy of GeoSmart

{ 11 trackbacks }

France Twitted by chel_elizabeth from Nord-Pas-de-Calais, France
August 1, 2009 at 6:57 pm
United States 17 PHP Practices That Should Be Banished Forever from Minnesota, United States
August 2, 2009 at 5:52 pm
United States php-html.net from California, United States
August 3, 2009 at 8:11 am
United States Ihc! « brunitto's weblog from Texas, United States
August 3, 2009 at 9:17 am
United States Samuel Folkes’ Blog: 17 PHP Practices That Should Be Banished Forever | PHP from California, United States
August 5, 2009 at 8:04 pm
Lithuania Verta paskaityti #2 from Vilniaus Apskritis, Lithuania
August 6, 2009 at 5:40 am
United States Samuel Folkes: 17 PHP Practices That Should Be Banished Forever | ChrisRenner.com from Georgia, United States
August 6, 2009 at 8:57 pm
United States Adrian Dvergsdal (atmoz) 's status on Saturday, 08-Aug-09 12:46:23 UTC - Identi.ca from United States
August 8, 2009 at 7:46 am
Germany Roundup: PHP Security | Veit's Blog from Bayern, Germany
August 10, 2009 at 3:41 pm
United States Peer Review: Looking Into Abstraction | BrandonSavage.net from Missouri, United States
August 26, 2009 at 12:02 am
United States Peer Review: Looking Into Abstraction – Brandon Savage | rapid-DEV.net from California, United States
August 26, 2009 at 3:05 am

{ 52 comments… read them below or add one }

1 United States Anonymous from California, United States August 1, 2009 at 6:51 pm

Most of these are common sense and make sense. But the “Not Closing Persistent DB Connections” makes very little sense. Persistent DB connections are just that, persistent. You don’t close them. You want to make sure you use the same credentials so you just have one per process open, but since they persist from one request to the next, you do not close them. You leave them open so the next request has an open connection available.

2 United States Sam from United States August 1, 2009 at 7:15 pm

Twitter: @SamuelFolkes

Ah, but common sense apparently isn’t so common is it? You are absolutely correct regarding persistent connections. That heading was supposed to be amended to remove the word ‘persistent’. I was actually making reference to connections opened with ‘mysql_connect()’ as opposed to ‘mysql_pconnect()’. Thanks for catching that for me…

3 Italy Giorgio Sironi from Liguria, Italy August 3, 2009 at 4:37 am

> 9. Unnecessarily Wrapping Native Functions in Classes
I do not agree. Of course I will write a class wrapper for curl if needed; the unique way to do unit testing is to mock out collaborators by passing in a subclass that returns canned result instead of the real object, so if I want to test my GoogleSearch class that uses curl functions I will have to write a simple Curl one to be mocked in testing; or I’ll become unable to test my application without depending on an Internet connection (with latency, connectivity and so on).
The other points are well written, register_globals and magic_quotes are now deprecated and scheduled to be removed from php 6.

4 Belgium Koen from West-Vlaanderen, Belgium August 3, 2009 at 5:09 am

Note nr. 17 and difference with

foreach (a_function($array) as $key => $value) {}

5 United States Brian Reich from Pennsylvania, United States August 3, 2009 at 7:13 am

Regarding “Recursive Inclusion”, its certainly “ugly” to have a long list of require_once() calls at the beginning of a file, however in some projects I still do it. Sometimes it feels worth the trade-off to explicitly list and track a particular file’s dependencies. If anyone knows of a better way to track dependencies using an autoloading solution, I’m all ears :)

6 Canada Jef from Quebec, Canada August 3, 2009 at 7:47 am

This is a nice article, should be in the toolbox for every beginner (and god knows there is a lot of people with insufficiant knowledge who’s writing PHP). And for the advanced one, a “spotcheck” about their coding methods is always welcome! Thanks!
Jef

7 Norway Dagfinn Reiersøl from Oslo, Norway August 3, 2009 at 8:25 am

I think there are often good reasons to wrap native functions. The most important may be the ability to use mock objects to test the client code without having to run the actual functions.

8 United States mgroves from Ohio, United States August 3, 2009 at 8:46 am

Using short “asp style” tags is stupid because PHP is not ASP? That’s the reasoning?

9 Canada julien from Quebec, Canada August 3, 2009 at 8:54 am

you mean short tags like these? :

10 Canada julien from Quebec, Canada August 3, 2009 at 8:55 am

euhm like THESE? : <? ?>

11 United States Jacob Santos from Missouri, United States August 3, 2009 at 8:57 am

Ah, but mixing languages in the same file is part of PHP’s advantage. What I guess you mean is not mixing business logic with presentation. Attempting to use a ‘template’ language is to undo the entire purpose of PHP. Although, it is always a good idea to separate CSS and JavaScript from the main HTML, there is no hard and quick rule for doing so.

As it is a matter of preference with some reasoning. I would merely point out that template languages that you would use to not use PHP in mixed with HTML is not worth the effort. I will also say that HTML often should not be outputted by PHP, so I believe the gist of what you said is correct, it is just not worded in a way to not confuse novices.

12 South Africa Foxinni from Gauteng, South Africa August 3, 2009 at 9:02 am

Awesome post. I’ve been know to use one of two of these in my code. Luckily I’ve tough myself not to use many of these. :)

13 Jamaica Sam from Saint Andrew, Jamaica August 3, 2009 at 9:57 am

Twitter: @SamuelFolkes

@Giorgio – You make a very good point. However I doubt that your Google search class would only use cURL functionality. What I hava huge problem with is writing a wrapper class exclusively for cURL. That only serves to rob cURL of some of its goodness i.e. its speed.

@Brian – You’ve got me there. I’ve never really had to keep track dependencies on a per file basis but one day I may need to. Maybe we should work on creating a solution :D

@Dagfinn – Agreed, however please see @Giorgio.

@mgroves – Yes, indeed. There are of course other seemingly more valid reasons such as the fact that using asp style tags depends on a php.ini setting or that it is deprecated and marked to be removed from the language entirely. However I think that the biggest reason that asp style tags are silly is that PHP is not ASP. ASP is a competing language that is completely different from PHP.

@Julien – Yes, those are short tags.

@Jacob – You are right. I too don’t particularly fancy templating languages and PHP should not be used to output HTML where doing so can be avoided. I didn’t mean that every bit of PHP should be removed from a file containing HTML but as you pointed out maybe I didn’t articulate what I meant well enough. Business logic is what needs not be mixed with HTML.

14 Germany Sascha Kimmel from Nordrhein-Westfalen, Germany August 3, 2009 at 2:20 pm

Good article, though closing non-persistent database connections, e.g.MySQL connections doesn’t make sense because PHP does that automatically for you:
“Using mysql_close() isn’t usually necessary, as non-persistent open links are automatically closed at the end of the script’s execution. ”
http://de.php.net/en/function.mysql_close

15 India Bijay Rungta (@rungss) from West Bengal, India August 3, 2009 at 3:04 pm

Excellent Points…

I wish People started understanding the severity of these Points and started to not use it in their Code.

But Sadly I don’t really see it…

These will end only with latest versions of PHP being used where some of these are not supported at all…

@rungss on Twitter

16 Netherlands alfred from Overijssel, Netherlands August 3, 2009 at 4:30 pm

Thanks for this list.

Still I think some points I don’t think I can agree with

9. I like to wrap almost everything in classes because of the damn namespace collision like for example zend framework does. (For example Zend_Controller, Zend_Layout, Zend_Config, Zend_Db, Zend_Db_Table, Zend_Registry). I know that functional programming has better performance….
12. Persistent connections are faster then non-persistant connections when you have enough memory because there is an overhead to creating tcp/ip connections.

http://talks.php.net/show/phpuk07/10

17 Jamaica Sam from Saint Andrew, Jamaica August 3, 2009 at 8:03 pm

Twitter: @SamuelFolkes

@Sascha – That is true. However, aside from the fact that I think its just good practice to close things when you open them rather than relying on someone else to, You can shave a few milliseconds off the execution of your script as well as precious system resources if say you open a single database connection at or near the beginning of the script and close it when you’re done rather than waiting for PHP to close it for you.

@alfred – Thank God for namespaces in PHP 5.3 huh? You’re right about persistent connections being faster, I don’t dispute that. However, if the connection isn’t persistent then you should probably close it.

@Bijay – Thats why I am so excited about and thankful for the proposed changes in PHP 6

18 United States Brian Reich from Pennsylvania, United States August 3, 2009 at 8:34 pm

Sam,

Despite some of the criticism I think you’re article was still very good; though what was even better was the dialogue that it invoked. It’s interesting that, while most most people agree that the majority of these practices should go away, folks can make a very compelling case as to when and why some them should still be used. At least we can all agree that dependence on register_globals needs to go away :)

19 Jamaica Sam from Saint Andrew, Jamaica August 3, 2009 at 8:45 pm

Twitter: @SamuelFolkes

Thanks Brian. As developers we need to promote healthy discussions amongst ourselves, it can only help us to learn and get better. I never would have laid any thought to tracking dependencies while using an autoloader had you not posted your comment. So agree or disagree we all need to speak up more. Register globals certainly needs to disappear. We’ve moved way past that, I wince whenever I see code snippets utilizing it.

20 India Sunny from Maharashtra, India August 3, 2009 at 10:31 pm

Nice share, thanks.
Point no. 11 “can accomplish some pretty amazing things as demonstrated in this excellent article” link is pointing this article only.

21 Jamaica Sam from Manchester, Jamaica August 4, 2009 at 12:02 pm

Twitter: @SamuelFolkes

Thanks for pointing that out Sunny, I’ve updated the link to point to the correct article.

22 Netherlands alfred from Overijssel, Netherlands August 4, 2009 at 12:07 pm

@Sam you are right to close the mysql connection as soon as possible if not using persistent connection, because for example low memory etc … Good list though

23 Bangladesh Mahmud Ahsan from Dhaka, Bangladesh August 5, 2009 at 2:38 pm

cool!

24 Peru Jonathan O. Nieto from Lima, Peru August 5, 2009 at 2:40 pm

Nice advices to follow in any project, regards!

25 Netherlands Arnoud from Overijssel, Netherlands August 5, 2009 at 2:50 pm

Regarding the recursive inclusion, you might need to re-consider the performance impact of using an autoloader. Sure your autoload loads classes just before being used and not a moment sooner, but having your include statements correctly positioed saves you the translating classnames into paths and all kinds of lookups and transformations all together.
The larger problem lies in the tendency to scatter each class in a single file, eventhough several groups of classes often are mostly used in strong conjunction and always loaded in sets. This could be fixed by build/deploy scripts (ANT/Maven/Phing) to optimize your code (including frameworks and libraries) without having too much impact on your development time processes; That however is just not a common practice for the PHP community.

26 United States Oscar from Maryland, United States August 5, 2009 at 3:20 pm

#15 is bad because somewhere you could do a define(‘boy’, ‘girl’); and then the php interpreter would be looking for $myArray['girl']

27 Jamaica Sam from Saint Andrew, Jamaica August 5, 2009 at 4:18 pm

Twitter: @SamuelFolkes

@Arnoud – I can see where translating an existing system to use a particular class naming scheme could prove cumbersome. However if the system is being designed from scratch it would certainly be a good idea for the developer(s) to define a standard nomenclature for classes and essentially all the files that sre a part of the application. It is very important to code to a standard. The standard itself doesn’t really matter, as long as it adheres to what is generally considered to be good coding practice. Having a standard for writing your code would make the implementation of an autoloader much easier. As for performance hits, it really just depends on how your code is structured. In many cases defining and using an autoloader actually reduces the load on your CPU. Take a look at this article for some actual numbers.

28 United States Jestep from California, United States August 5, 2009 at 9:45 pm

My definition for this would be “Don’t be a lazy programmer”.

I think so much of this is prevalent because PHP has many entry level and programmers with no formal training. Stuff like this is why PHP has a wrap as a crappy language by many.

Here’s 2 more practices that drive me crazy. Using “global $variable;” because you’re too lazy to learn about scope or design and use a proper interface. And not checking the php manual.

29 Germany derschreckliche from Baden-Wurttemberg, Germany August 6, 2009 at 4:27 pm

Very nice article, thanks for that!
There is a funny thing about the link in Nr. 12 (the singleton):
It’s a good working implementation but it could use __autoload() as a better practice, like you wrote at Nr. 8 ;)

30 Brazil Rafael Izidoro from Sao Paulo, Brazil August 7, 2009 at 7:13 am

Excelent article…

31 Bangladesh saiful103a from Dhaka, Bangladesh August 7, 2009 at 2:28 pm

this will help me a lot.thanks for your nice article.

32 United Kingdom Paul Curle from Hertford, United Kingdom August 7, 2009 at 7:49 pm

I am a newbie to PHP but a long time programmer so this is an important post as far as I am concerned
1. Using Register Globals. I downloaded PHP and off I went. I’m using V5, so I should be OK, but I’ll check to make sure.
2. Relying on Magic Quotes. I saw the addslashes() and stripslashes()functions being used all over the place while I was looking at stuff and wondered what it was about. I know now!
3. Not Sanitizing and Validating User Input. In my opinion the first rule of writing software for someone else is “Every user is a potential idiot”. I think 95% of software is catering for idiots. 4% is probably doing something and 1% is fun for someone, somewhere, in a galaxy far, far away… I digress.
4. Mixing HTML, PHP, SQL, JS, CSS, Etc, Etc. Got to agree with you, but it is SO easy with HTML & PHP and sometimes the pain of separation is too much. Maybe there is no ‘must’, but when you trawl the WWW without setting a date on the results you see something from 199? that works. And then………
5. Using Short/ASP Style Tags. It wasn’t me!! I never did it!
6. Not Documenting Code. Documentation in a commercial environment is a must. Documentation is the first thing to go out of the window when timescales are squeezed. Verbose code helps – variables having meaningful names instead of x and y – but just one sentence of what is supposed (!) to happen can be a great help. – especially to the revisiting developer!
7. Using User Input as an Argument in eval(). Managed to avoid eval() myself so I agree with Lerdorf.
8. Recursive Inclusion. I didn’t know about either of the __autoload() magic function or the autoloading features of the Standard PHP Library (SPL). I do now. Thank you.
9. Unnecessarily Wrapping Native Functions in Classes. You are right
10. Using die() to ‘Handle’ Errors. Error handling is needed to let the user down gently when they have been stupid and to give the fixer a clue where to start when something has gone really, really wrong. So give meaningful messages.
11. Using Regex Functions for Basic String Manipulation. Why show off?
12. Not Closing Database Connections. Finished? Then clean up!
13. Error Suppression Using The ‘@’ Symbol. See 10.
14. Inappropriate Display of Errors. I was that amateur once. If you write it then get someone else to be the idiot to test it or at least try it out. As the coder, you know how it works and can avoid the errors – deliberately sometimes.
15. Unquoted Array Keys. I must have hit the right web page on this one. Phew! Lucky me.
16. Using Uninitialized Variables. PHP is not he only laid back language. I hate coming across “x” on line 221 for the first time (this does not mean that any other line number is OK. You know what I mean). I like to know what “x” is a supposed to represent.
17. Placing Functions in Loop Declaration. I have caught myself out on this. It is easy to do and just as easy to avoid. Loops of any sort should always be controlled by a recognizably finite value.

33 Egypt ???????? from Al Qahirah, Egypt August 9, 2009 at 6:44 am

The subject of more than a wonderful and useful

34 Netherlands Arnoud from Overijssel, Netherlands August 10, 2009 at 7:05 am

@sam
The article you mentioned , has some nice numbers in it, but when looked at the situation, the development just seemed to have spun out of control. If you need autoloading to make sure that redundant code is no longer included and parsed, and then talk about increasing performance, you might be on the wrong track anyhow.
One thing I am wondering about is the number of include_once calls you can do to a single class file, to have it be less performing than an autoloader solution.

35 Germany xodarapsad from Schleswig-Holstein, Germany August 10, 2009 at 2:04 pm

I was kinda thinking, okai, thats for newbies.

But I still do 17. Placing Functions in Loop Declaration sometimes and forget to clean it.

Comes in my short-term to long-term memory list.

36 Australia William Shields from Western Australia, Australia August 12, 2009 at 5:11 am

I mostly agree but some of the points I strongly disagree with:

4. There are so many ways this one is wrong it’s not funny. I’ll pick just one: If you want a page that loads quickly you will load all your Javascript into a separate file (as functions) with a far future Expires header. Then, on your page, you will call the functions you need for that page so you don’t execute unnecessary code.

Also, not mixing PHP and HTML?!?! What the…? I can only assume the implicit recommendation is to use a templating system like Smarty, which I have never seen the point of. Why use a templating language on top of PHP, which is also a templating language?

5. Short tags: I use them all the time. I don’t write XHTML (there’s no point–even XSLT processors can output HTML). I write HTML. I don’t really care that <? is an XML PI construct.

13. For some things you should use @ error suppression like when deleting a file you're not sure is there. @unlink($filename) makes perfect sense because the "error" isn't really an error.

17. This is micro-optimization. Not that I'm calling the suggest variant bad. It's just a pointless distraction (99% of the time) to perform this kind of "optimization".

37 United States Brian Reich from Pennsylvania, United States August 12, 2009 at 2:05 pm

@ William Shields,

All valid points, although what you said regarding separating JavaScript into a separate file for faster loading sort of proves Sam’s point.

I think there is a happy medium to be found between “not mixing PHP and HTML” and not over-thinking the problem by using a template system. I agree with you that it’s silly to use a system like Smarty on top of PHP… its redundant, over-complicated, and completely unnecessary. Personally I like to follow the MVC design pattern, which keeps my logic in one location (usually a Controller class written in PHP), and my view in another (also written in PHP/HTML). The only PHP mixed into the view scripts outputs cleaned data, or loops through data to be rendered. When used in this fashion PHP is just as simple a template engine as Smarty or most others.

38 Jamaica Sam from Saint Andrew, Jamaica August 12, 2009 at 2:21 pm

Twitter: @SamuelFolkes

@William – I believe the ideas behind some of these points have escaped you, particularly the way you have interpreted number 4. When I say don’t mix the languages I don’t mean that they should all be entirely exclusive of each other. Clearly you’ll need to include javascript function calls in your XHTML documents if you have external .js files. The idea behind the point is that its is bad practice to have one file containing PHP logic, javascript logic, CSS styles, SQL queries and XHTML markup. That leads to very tight coupling (yes I am using the term outside of an OOP context because is applies) between the various components of your application. It is very ugly and sets the stage for duplicating your code. I hate templating languages by the way.

You don’t write in XHTML but many other people do. If you ever want to share your code you’ll run into problems. You clearly aren’t considering the implications of using short tags in an enterprise development environment with multiple developers. Also, I expect that you’re preparing to use PHP5 forever because short tags are meant to be deprecated in PHP6.

Every error is an error. There is no such thing as ‘an error that isn’t really an error’. I will concede that it is very handy to suppress the error but an alternative that excludes the use of the very expensive error suppression operator would be something like this:

if(file_exists($filename)) unlink($filename);

Simple and clean. Still one line of code.

Point #17 is NOT a micro optimization. Again, when you start developing in an enterprise environment and you’re looping over 500,000 iterations with function calls in the loop declaration you will DEFINITELY notice a performance hit.

Some of these bad practices you’ll get away with in a small application because as I stated PHP is very forgiving and the interpreter does not complain very much. When you try to expand your app however you’ll find that it doesn’t scale well. You start getting complaints from te interpreter. You start seeing your app taking years to load. If we practice coding the right way from the start we don’t set ourselves up for headaches in the future.

39 Jamaica Sam from Saint Andrew, Jamaica August 12, 2009 at 2:23 pm

Twitter: @SamuelFolkes

@Arnoud – I see your point. I guess that’s where the tradeoff between convenience and efficiency arises…

40 United States Brian Reich from Pennsylvania, United States August 12, 2009 at 8:34 pm

@Sam from Jamaica:

While I completely agree in theory with what you’re trying to say the example you gave is a little bit flawed. There are still a multitude of reasons why unlink might fail, even after verifying that it exists.

if(file_exists($filename)) {
    unlink($filename);
    /*
     * Could still fail if:
     * $filename is read-only
     * PHP process doesn't have permission
     * $file is a non-empty directory.
     */
}

Should you check all cases in which this call might fail? Or should you simply try unlink, let it fail, and handle the error?

41 Jamaica Sam from Saint Andrew, Jamaica August 12, 2009 at 10:35 pm

Twitter: @SamuelFolkes

@Brian – You are quite right, slight oversight on my part. I still feel that checking all the cases or, as you suggested, handling the error, are better alternatives than suppressing it. I could argue that error suppression is expensive from a performance perspective and you migth reply by labelling by categorizing that thought as a micro-optimization. I would agree with you but suppressing errors just *feels* wrong. I believe that errors should always be handled properly rather than hidden…

42 Germany Dominik from Baden-Wurttemberg, Germany August 13, 2009 at 11:39 am

Oh wow. I have to admit, I came here with “oh, no, anoter ‘x Things about PHP you need to know’ post” in the back of my head and was ready to do some bashing, because most of those are really bad.

But no.

I have to agree with all of the above. Some of them should be the very basics – but not necessarily for the average beginner.

About 13: I usually have all error reporting off on production systems, which makes this is a non-issue. I don’t even need to put @ to avoid warnings. Just make sure that in case of REAL errors (not warnings) someone gets notified that something’s wrong.

43 Ireland artur ejsmont from Dublin, Ireland August 16, 2009 at 4:35 pm

indeed a nice list :- ) especially @ operator is a thing that is killing me as well as not logging errors or not using exceptions where applicable or swollowing them in empty catch.

Another little annoyance is overusage of static methods and variables as you cant mock it.

Also dont overcomplicate (the things people do with operators and loops …. my god)

And last thing from top of my head is adding get/set methods or even worse transparent accessors to method private data ….. it makes everything like it was public. expose only what is necessary and in a way people can use it!

there should be more posts like that maybe people would not code like it :- )

cheers

44 United States Reid from Oregon, United States September 3, 2009 at 1:54 pm

Agreed agreed agreed on all points except one (and it seems I’m not alone) — “Unnecessarily Wrapping Native Function in Classes”. What’s great is your comment too “Guys c’mon! Stop writing wrappers for cURL!”….because that’s one of my favorite wrapper classes! I’m a fan of semantic programming. I’m also a fan of being able to write code w/o having to look up every single time what the flags for this that or the other are. So in this case…cURL…I think it lends itself especially well to being wrapped in a nice, clean class. Bloated? No. Not. At all. Just cleeeeeean, readable, and most of all simple to use. Naturally YMMV (and obviously does).

Great article though, thanks.

45 United States Peter Ubriaco from New York, United States September 4, 2009 at 5:25 pm

In example 17 the problem is not that there is a function in the loop declaration. The performance problems only occur when an expensive function is called on each iteration. Another way to solve this is to include a function in the loop declaration’s first expression, which will not incur the performance penalty, and that is:


for ($i = 0, $n = count($myArray); $i < $n; $i++)
{
//Code
}

46 United States Richard Lynch from United States September 8, 2009 at 11:32 am

Twitter: @LynchRichard

The idea that you would recommend addslashes for DB escaping in 2009 in an article like this is even more scary than the original 17…

Use the database-specific escaping (mysql_real_escape_string, for example) that will take the charset into account!

I hope I am not as disappointed by 3-17 as I was by #2…

47 United States Richard Lynch from United States September 8, 2009 at 11:37 am

Twitter: @LynchRichard

#16, the short version:
Use error_reporting(E_ALL) and fix your damn bugs. :-)

48 United States Reid from Oregon, United States September 8, 2009 at 11:56 am

“The performance problems only occur when an expensive function is called on each iteration.”

Not true. Even a simple function, one such as ‘count’, becomes expensive when the number of loop iterations becomes large. But even for a small number of iterations, calling a function more than a single time is simply lazy programming practices. It appears you agree, as your code sample would indicate – you are doing exactly what the author suggests.

49 Spain Alberto Toquero from Pais Vasco, Spain September 18, 2009 at 2:41 am

I am a newbie to PHP,
I’d be thanked you if someone give an example (bad practices and the good solution) of points 1 and 2 ?

50 Canada Djalil from Quebec, Canada September 22, 2009 at 3:10 am

I did not even know that it is possible to use unquoted array keys ;)

Leave a Comment

You can use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong> <pre lang="" line="" escaped="">

Previous post:

Next post: