PHP beginner - looking for veteran criticism of code example (code works btw, that's a start!)

Question

From what I've gathered browsing SO, I'm the worst nightmare of good people like you: someone who never formally learned any programming, decided he'd like to enhance his hobby website, and bought a fast-track PHP book off the shelf and went at it.

Browsing SO has taught me a great deal of things, highlights of which include:

• PHP is terrible for beginners (of which I am)
• PHP is a terrible first language (it's my first)
• lots and lots of PHP code is awful (mine probably is)
• most people have no clue how to template in PHP (my book recommends Smarty <grin> )
• functions are awesome, if you have a clue (I don't)

In light of reading pages and pages of veteran coder ramblings, I have become perhaps the world's most paranoid and self-conscious beginner.

Which is why I now prostrate myself before you, offering up my firstborn child... err, chunk of working code, and humbly beg that you tear & criticize it to pieces, that I may more expeditiously learn the error of my ways...

The goals of the following code:

1) Grab a list of units from table unit_basic_data based on their nationality, returning their unitid for query purposes and their unitname for display to user.

2) Build a form with checkboxes for each unit in this format

`<tr><td><input type="checkbox" name="Americanunit[]" value="$unitid" />$unitname</td></tr>`

You'll note that "American" is currently manually set to $nation and then appended to unit[] -- this is to allow me to add a dynamic country select later.

3) For each checked box, submit an update query to table scenario_needunits composed of the scenario id scenid and unit id unitid, which taken as a pair are a primary key.

4) Dump out a rough list of unitids updated to the user and a confirmation of completion message.

What the code does now: Everything! :-) Except in a rather ugly fashion.

Obvious problems: no sanitization (btw, do checkboxes require sanitizing?), no error reporting if user tries to submit a scenario|unit pair that's already in the table

Further questions to the experts:

• Is there a cleaner way to build the checkbox strings besides my approach that concatenates SIX variables?
• Is this a templating abomination?
• Can it be split into multiple files? (if yes, how/where?)
• Am I already hopeless?

Aside from the above, I am 100% open to any other comments & criticism. I thank ALL of you most kindly for your time, and your patience. And I promise the next bit of code I post to SO will look a lot nicer!

Code follows:

<?php
// designed to dynamically load a country's unit list from the database
// and allow you to submit the country's units needed for a scenario
error_reporting(E_ALL);
require_once 'login.php';
$db_server = mysql_connect($db_hostname, $db_username, $db_password);

if (!$db_server) die("Unable to connect to MySQL: " . mysql_error());

mysql_select_db($db_database)
or die("Unable to select database: " . mysql_error());

$nation = "American"; // desired nation
echo "$nation is selected.<br />";
$sql = "SELECT unitid,unitname FROM `unit_basic_data` WHERE `forcename`='$nation'"; // grab all current units for $nation
$result = mysql_query($sql);
if (!$result) die ("Database access failed: " . mysql_error());
$rows = mysql_num_rows($result);
echo "Total rows returned is $rows rows."; // display total number of units
$formOpen = '<form method="post" action="scenario-needunits2.php"><input type="hidden" name="submittedCheck" value="yes" /><br />';
$scenario = '<tr><td>Scenario Name: <input type="text" name="scenid" /></td></tr>'; // input target scenario for new data
$checkPre = '<input type="checkbox" name="'; // checkbox string part 1 of 5
$checkName = $nation; // checkbox string part 2 of 5
$checkMid = 'unit[]" value="'; // checkbox string part 3 of 5
$checkPost = '" />'; // checkbox string part 5 of 6
$row = ''; // checkbox string parts (4 of 6) & (6 of 6)
$checkSubmit = '<br /><input type="submit" /></form>'; // submit button & close form
echo "<table><tr><th>Units</th></tr>";
echo "$formOpen";
echo "$scenario";
for ($j = 0 ; $j < $rows ; ++$j) // for statement that returns result row by row
{
 $row = mysql_fetch_row($result);
 echo "<tr>";
 echo "<td>$checkPre$checkName$checkMid$row[0]$checkPost$row[1]</td>"; //brutal, but effective
            echo "</tr>";
}
echo "</table>";
echo "$checkSubmit<br />"; // form closes here

if (isset($_POST['submittedCheck'])) //begin table update code
    {
        $scenid = $_POST['scenid']; // grab scenid
        $americanunit = $_POST['Americanunit']; // grab unit array
        foreach($americanunit as $unitid) // run through array
        {
            $sql = "INSERT INTO scenario_needunits VALUES ('$scenid' , '$unitid');"; // create query string
            $result = mysql_query($sql); // add one new row
            echo "Inserted a $unitid for scenario $scenid<br />"; // report addition
        }
        echo "Complete."; // report update has been successful
    }
?>

Answer 1

There are four things that spring to mind:

1. The code is hard to read;
2. There is no separation of presentation and processing logic;
3. Outputting HTML this way is probably my least preferred method; and
4. Your code is susceptible to SQL injection.

Here are my suggestions.

1. Split your code into processing logic (called a "controller") and presentation (called a "view");
2. Construct HTML elements with functions where it makes sense as this can be much easier to read; and
3. Always treat user input as suspect for the purposes of defending against SQL injection and XSS (cross-site scripting) exploits; and
4. Put common code into an included file.

For example:

config.php

<?php
error_reporting(E_ALL | E_STRICT);
$db_server = mysql_connect($db_hostname, $db_username, $db_password);
if (!$db_server) die("Unable to connect to MySQL: " . mysql_error());
mysql_select_db($db_database) or die("Unable to select database: " . mysql_error());
?>

pagename.php

<?php
require 'config.php';
require 'login.php'; // i assume this checks to see they are logged in?

if (/* user pressed submit */) {
  // do some processing
  if (/* it worked */) {
    $message = 'Success';
  } else {
    $message = 'Failure'; // possibly with explanation why
  }
} else {
  // initialize some values
}

require 'viewname.php';
?>

viewname.php

<form method="post">
<?php echo create_checkbox('name', true /* checked */); ?>
<table>
<?php foreach ($rows as $row): ?>
<tr>
  <td><?php echo create_checkbox(...); ?></td>
</tr>
<?php endforeach; ?>
</table>
</form>

The logic in pagename.php is a crude controller in the MVC (Model-View-Controlelr) sense. MVC is a common UI pattern. Some will argue you "need" a framework (CodeIgniteger, Kohana, Zend, CakePHP, Symfony, etc) but you don't. They have various other benefits but the above is a simplistic (and often sufficient) split of the view and controller. The view should largely just be a template that takes data given to it.

The above only has one view but you could easily branch in the controller to call multiple views.

Template functions can really help on code readability. For example:

function create_checkbox($name, $checked = false) {
  $ch = $checked ? ' checked' : '';
  return <<<END
<input type="checkbox" name="$name" id="$name"$ch>
END;
}

This can be called in your views/templates and will be much more readable. If you have a function that has more than say 2-3 arguments pass in an array of options instead:

function create_checkbox($options) {
  $checked = $options['checked'] ? ' checked' : '';
  $class = $options['class'] ? ' class="' . $options['class'] . '"' : '';
  $id = $options['name'];
  $name = $options['name'];
  return <<<END
<input type="checkbox"$id$name$class$checked>
END;
}

with:

<?php echo create_checkbox(array('name' => 'cb1', 'checked' => true, 'class' => 'cbclass')); ?>

Lastly, never accept user input like this:

$field = $_POST['field'];
$sql = 'INSERT INTO tablename (blah) VALUES ('$field');
mysql_query($sql);

That is a huge security hole (creating a vulnerability to an attack called "SQL Injection"). Always use mysql_real_escape_string():

$field = mysql_real_escape_string($_POST['field']);
$sql = 'INSERT INTO tablename (blah) VALUES ('$field');
mysql_query($sql);

To avoid XSS you will often want to strip tags out of user input. Th easiest way to do this is:

$field = filter_var($_POST['field'], FILTER_SANITIZE_STRING);

There are different filters and validations you can do with this. See filter_var() and the types of filters.

Answer 2

Some tips

1. Use PDO to access your database (Pro's and con's of using PDO)
2. Familiarize yourself with MVC
3. Read some books on programming practices I personally suggest Code Complete and Pragmatic Programmer

Answer 3

Indentation. The code above is just impossible to read; in a couple of months you will find it impossible to maintain.

String slinging. PHP is a templating language. Use it, don't fight it. When you laboriously piece together HTML strings you've crafted by hand you're missing the point. Use PHP's own <?php interpolation ?> in preference. Combine this with the indentation so you have one, single, well-formed hierarchy.

String escaping. Forget “sanitisation” for a minute, because it's not the real solution. Sanitising input is a domain-specific function you can apply to certain parameters to make sure they fit your model, not something you should hack across all submitted parameters to try to “get rid of bad characters”. There is no such thing as a bad character, only one you've handled inappropriately.

When you concatenate raw text into a non-raw-text context like HTML, whether through $html= "<blah>$name</blah>" or <?php echo ?>, you need to escape it into a format appropriate for HTML. That means calling htmlspecialchars on it.

When you concatenate raw text into an SQL statement, you need to escape it into a format appropriate for SQL. That means calling mysql_real_escape_string before wrapping it in single quote. It doesn't mean rejecting all strings containing single quotes or removing single quotes; to a person named O'Reilly, that apostrophe is very important.

(For SQL in particular, you should also consider parameterised queries, which stops you having to worry about escaping completely, or even a higher-level data access layer.)

So the view part might look something like:

<?php
    // Shortcut to htmlspecialchars for convenience.
    // Would normally put in an include library.
    //
    function h($s) {
        echo(htmlspecialchars($s, ENT_QUOTES));
    }
?>

<?php
    // Get a country's unit list
    //
    $units= mysql_query(
        "SELECT unitid, unitname FROM unit_basic_data ".
        "WHERE forcename='".mysql_real_escape_string($nation)."'"
    );
?>

<form method="post" action="units.php">
    <table>
        <tr>
            <td>
                <label for="f-scenid">Scenario name</label>
            </td>
            <td>
                <input type="text" name="scenid" id="f-scenid" />
            </td>
        </tr>
        <?php while($unit= mysql_fetch_assoc($units)) { ?>
            <tr>
                <td>
                    <label for="unit-<?php h($unit['unitid']) ?>">
                        <?php h($unit['unitname']) ?>
                    </label>
                </td>
                <td>
                    <input type="checkbox"
                        name="<?php h($nation) ?>unit[]"
                        value="<?php h($unit['unitid']) ?>"
                        id="unit-<?php h($unit['unitid']) ?>"
                    />
                </td>
            </tr>
        <?php } ?>
    </table>
    <div>
        <input type="submit" value="Submit" />
        <input type="hidden" name="action" value="insert" />
    </div>
</form>

Answer 4

Wow. Pretty good responses!

Answer 5

my two cents:

• PHP is terrible for beginners (of which I am)

PHP is a perfectly fine language for beginners or anyone else. It's well-suited to web programming.

• PHP is a terrible first language (it's my first)

PHP is a perfectly fine first language. Because there are so many built-in functions, as a beginner you can do a whole lot in a few lines of code. It makes you feel good, reinforces the fun factor. Later on, you will probably want to borrow a Java book from someone to get more serious about object-oriented programming and unit testing. A lot of the good ideas in the Java world have been ported to PHP.

• lots and lots of PHP code is awful (mine probably is)

Lots and lots of code in every programming language is awful. Yours probably is, but it's no big deal. The more code you write, and the more code you read, the better your code will get, provided you work at it. It's the same as learning to write well in English.

• most people have no clue how to template in PHP (my book recommends Smarty )

Smarty is actually a pretty nice templating library. I wouldn't focus on this too much. PHP programming has almost nothing to do with templating.

• functions are awesome, if you have a clue (I don't)

One step at a time. Functions are a way of organizing thought by breaking a computation into pieces. Objects, which contain functions and object-scoped variables, offer a higher level of organization; design patterns, which describe ways of organizing groups of related objects, an even higher level. Again, it's like learning to write: structuring thoughts into coherent sentences is one thing; coherent paragraphs, chapters, books--that comes later, and you can't learn it all at once.

The main thing is reading lots of other peoples' code to start to develop a sense of style, not being afraid to break other peoples' code or write sloppy code of your own, and just keep at it. newbz ftw!