Three lessons from amateur PHP code

May 11, 2014

Photo CC-BY by tnarik 

I did not get a CS degree and thus learned web development as a self-taught amateur. While that was fun, it also meant that in the process of building things without a curriculum or a mentor to provide a foundation I wrote pretty bad code in the beginning.

I have dug up the PHP code for a site which has been superseded with a Drupal site since 2009 and it’s a great way to show failure by example without anyone getting hurt.

The following shows common mistakes made by amateur PHP programmers and a few pointers on how to fix them and become a better developer.

Mistake #1: Repeating yourself

Every time you see redundant code it should give you pause. Even a decade ago I recognized this and tried to move the page header into a separate file. Here is the start of item.php and dozens of other files like it:

<?php
$pagetitle="Item Details";
include "main.php";
echo "<div id=\"content\">";
// [...]

Viewing an item would thus be the URL /item.php?id=123. This structure exemplifies that the author has not thought enough about inheritance and overall structure. Furthermore, we can see in main.php that the file is just dumping content to the output like the all other PHP files in that application:

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<?php
include "settings.php";

It’s clear that the author has not even fully internalized procedural programming and is using includes simply to reduce redundancy in a static, linear script.

Fixing the mistakes

Point all requests to index.php as the sole entry point and use parameters to call the respective logic. Not only will you reduce errors but you can also centralize parameter checking and sanitization, as well as authentication and authorization. You can also easily add nice, readable URLs with a one-liner in htaccess.

Convert the static pages such as item.php into functions. Which is basically a given, if you restructure everything around your index.php as recommended above. In this case if we had all item.php logic wrapped in a function (or more likely a collection of them), we could call it where required through our routing logic and not attempt to build a linear script for each page. Ideally, you would follow established patterns like MVC for your program structure when doing so.

**Don’t solve this yourself. **Seriously consider if your homebrew application should continue to exist. The above points explain how one would fix something like it if you cannot ditch it but consider carefully if a framework such as Symfony2 or a CMS like Drupal is not the better solution to start from scratch, since they provide all that essential work of handling I/O, routing pages, abstracting templating, etc. for you, if you invest the time to learn how these systems work.

Mistake #2: Mixing logic, data and display

Here is an additional excerpt from the item.php shown above:

if ($row[isbn] != NULL)
echo "<tr><td>ISBN/ISSN #:</td><td> $row[isbn] </td></tr>";
echo "<tr><td>Section:</td><td> $row[section]</td></tr>";
// [...] 20 more <tr> like that
$commentquery = mysql_query("SELECT comment FROM comments WHERE item='$row[item_id]'");
if (!mysql_num_rows($commentquery)==0)
echo "<br /><br />Comments by members about this item:";

while($row = mysql_fetch_array( $commentquery )) {
    echo "<div id=\"comdiv\">";
    echo nl2br($row['comment']);
    echo "</div>";
}

Thankfully, the query parameter was properly sanitized, so it is not a case of little Bobby Tables but it is a Smörgåsbord of bad patterns. Not only is redundancy a concern but the indiscriminate mix of data, html markup and query logic shows missing levels of abstraction. The weird string double escaping we’ll just ignore.

Fixing the mistakes

Use PDO and prepared statements. If you write your SQL by hand and work with user input, you are asking for trouble. PDO tutorials abound.

Functions, functions, functions. The author should have seen the redundancy in his or her code and concluded that a helper function should deal with creating the table (let’s ignore whether a table is the right solution here). The block on comments could equally as well be encapsulated in a function. However, that function alone would not add much readability (at least as long as it is as short as it is now), unless one takes out the markup, which the following recommendation encompasses.

Create templates. Even if you do not follow the general advice from mistake #1 on using an established base to build upon, you can improve legacy code by stripping out the markup into templates. Even if you don’t want to fully commit to Symfony, you could use their Twig engine to do so and achieve much more readable and maintainable code. Again, consider not reinventing the wheel.

Mistake #3: Sloppy code conventions

if (!mysql_num_rows($commentquery)==0)
echo "<br /><br />Comments by members about this item:";

The line above we already saw in mistake #2. The problem here is that through missing brackets, spaces, and indentations the code becomes significantly harder to read. It’s easy to overlook the scope of the if-loop and for example incorrectly assume that the while-loop following it is still in it’s scope. The many formatting problems in the code were mainly due to the fact that I didn’t use software to help me stick to a standard, since I wrote it all by hand.

Fixing the mistakes

Use an IDE. A text editor with syntax highlighting is just not enough, it’s too easy to make mistakes. Your environment, whichever you decide upon, should do auto-formatting and code checking as-you-type. I use PHPStorm but the same can be achieved with free tools such as Eclipse.

Follow PSR-1/2. If you have until now followed your personal preferences, just get over yourself and standardize to PSR-2, that way all your code and all modern libraries look the same. There are also tools to help you convert your existing code.

All of the above will not make you into a great developer but I hope to have provided helpful advice to others that I wish I had been given at the time.