Good and Bad PHP Code

The following is republished from the Tech Times #165. When interviewing a PHP developer candidate for a job at SitePoint, there is one question that I almost always ask, because their answer tells me so much about the kind of programmer they are. Here’s the question: “In your mind, what are the differences between good PHP code and bad PHP code?” The reason I like this question is because it tests more than just a candidate’s encyclopedic knowledge of PHP’s functions. Zend’s PHP certification does a good job of that (as does the test that Yahoo! issues to applicants for its PHP developer jobs, apparently). Rather, the answer to this question tells me whether a PHP developer has, for example, experienced the pain of working with poorly-written code inherited from a careless predecessor, and whether he or she will go the extra mile to save the rest of the team from that same pain. I don’t have a set notion of the perfect answer to the question, but I do know the kinds of things I’m hoping to hear. Just off the top of my head: Good PHP code should be structured. Long chunks of code can be broken up into functions or methods that achieve sub-tasks with simple code, while non-obvious snippets should be commented to make their meaning plain. As much as possible, you should separate frontend HTML/CSS/JavaScript code from the server-side logic of your applications. PHP’s object oriented programming features give you some especially powerful tools to break up your applications into sensible units. Good PHP code should be consistent. Whether that means setting rules for the names of variables and functions, adopting standard approaches to recurring tasks like database access and error handling, or simply making sure all of your code is indented the same way, consistency makes your code easier for others to read. Good PHP code should be portable. PHP has a number of features, such as magic quotes and short tags, that can break fragile code when they are switched on or off. If you know what you’re doing, however, you can write code that works by adapting to its environment. Good PHP code should be secure. While PHP offers excellent performance and flexibility out of the box, it leaves important issues like security entirely in the hands of the developer. A deep understanding of potential security holes like Cross-Site Scripting (XSS), Cross-Site Request Forgeries (CSRF), code injection vulnerabilities, and character encoding loopholes is essential for a professional PHP developer these days. Once a candidate has answered this question, I usually have a pretty good idea of whether they’ll be hired or not. Of course, there’s always the possibility that an interviewee simply isn’t able to articulate these types of things, so we also have our candidates sit a PHP developer exam. Many of the questions in this exam seem straightforward on the surface, but they give candidates plenty of opportunity to show how much they care about the little details. The following “bad” code is a highly simplified example of the sort of thing we might put in our PHP developer exam. The question might be something like “How would you rewrite this code to make it better?”

<?
echo("<p>Search results for query: " .
    $_GET['query'] . ".</p>");
?>

The main problem in this code is that the user-submitted value ($_GET['query']) is output directly to the page, resulting in a Cross Site Scripting (XSS) vulnerability. But there are plenty of other ways in which it can be improved. So, what sort of answer are we hoping for? Good:

<?
echo("<p>Search results for query: " .
    htmlspecialchars($_GET['query']) . ".</p>");
?>

This is the least we expect. The XSS vulnerability has been remedied using htmlspecialchars to escape dangerous characters in the submitted value. Better:

<?php
if (isset($_GET['query']))
{
  echo '<p>Search results for query: ',
      htmlspecialchars($_GET['query'], ENT_QUOTES), '.</p>';
}
?>

Now this looks like someone we might want to hire:

• The “short” opening PHP tag (<?) has been replaced with the more portable (and XML-friendly) <?php form.
• Before attempting to output the value of $_GET['query'], isset is used to verify that it actually has a value.
• The unnecessary brackets (()) around the value passed to echo have been removed.
• Strings are delimited by single quotes instead of double quotes to avoid the performance hit of PHP searching for variables to interpolate within the strings.
• Rather than using the string concatenation operator (.) to pass a single string to the echo statement, the strings to be output by echo are separated by commas for a tiny performance boost.
• Passing the ENT_QUOTES argument to htmlspecialchars to ensure that single quotes (') are also escaped isn’t strictly necessary in this case, but it’s a good habit to get into.

Somewhat distressingly, the number of PHP developers looking for work that are able to give a fully satisfactory answer to this sort of question—at least here in Melbourne—are few and far between. We spent a good three months interviewing for this latest position before we found someone with whom we were happy! So, how would you do when asked a question like this one? Are there any factors that make PHP code good or bad that you feel I’ve left out? And what else would you look for in a PHP developer?

Frequently Asked Questions about Good and Bad PHP Code