3
\$\begingroup\$

I'm new to PHP programming. I would love some feedback on this simple code I wrote which queries a database based on some arguments supplied by a user and returns an HTML table displaying the data.

The table in my db has three columns: Manufacturer, Model, ForSale.

The user picks from three drop down menus on the web page and this php script is called with the data.

<?php
$db = new SQLite3("db/mantisui.db");

$manufacturer= $_GET['mfr'];
$model = $_GET['mod'];
$forsale = $_GET['forSale'];

// create the query
$q = "SELECT * FROM vehicles";
$addwhere = true;

// if all mfrs chosen, skip WHERE clause
$pos = strpos($manufacturer, 'All');
if ($pos === false) {
    $q .= " WHERE Manufacturer='$manufacturer'";
    $addwhere = false;
}

// if any models chosen, skip WHERE clause
$pos = strpos($model, 'Any');
if ($pos === false) {
    if ($addwhere == false) {
        $q .= " AND";
    }
    else {
        $q .= " WHERE";
        $addwhere = false;
    }

    $q .= " Model='$model'";
}

// if any for sale status chosen, skip WHERE clause
$pos = strpos($forsale, 'Any');
if ($pos === false) {
    if ($addwhere == false) {
        $q .= " AND";
    }
    else {
        $q .= " WHERE";
        $addwhere = false;
    }

    $q .= " ForSale='$forsale'";
}

$response = $db->query($q);

// generate the output table
$output = "<table id='screens' class='table tablesorter table-condensed table-striped table-hover table-bordered'>";
$output .= "<thead><tr><th>MANUFACTURER</th><th>MODEL</th><th>FOR SALE</th></tr></thead>";
while ($res = $response->fetchArray()) {

    $id = $res['Id'];
    $txtMfr= $res['Manufacturer'];
    $txtModel= $res['Model'];
    $txtForSale = $res['ForSale'];

    $output .= "<tr class='vehiclerow'><td style='display:none' class='id_row'>$id</td><td>$txtMfr</td><td>$txtModel</td><td>$txtForSale</td><td class='status_cell'>$status</td></tr>";
}

$output .= "</table>";
echo $output;

$db->close();
?>
\$\endgroup\$
2
  • 1
    \$\begingroup\$ See: xkcd.com/327 \$\endgroup\$ Jul 3, 2013 at 0:50
  • \$\begingroup\$ Ha! I'll try and remember that :) \$\endgroup\$
    – Chris
    Jul 3, 2013 at 16:18

2 Answers 2

3
\$\begingroup\$

Generally, it is a good idea to separate your view logic from your business logic, e.g your data access.

The code you've presented is procedural, and whilst there's nothing wrong with that at this stage, it could become unsustainable in terms of future maintainability.

One thing that I am conscious about, is that you are directly plugging variables into your SQL query that you are constructing: there is a risk of SQL injection. I would suggest that you wrap your variables with sqlite_escape_string before placing them into your query. I note that you might want to look into PDO for database portability and a 'better' layer between your application and the database.

You seem to be be using strpost to ascertain whether a manufacturer was 'All'. I would suggest doing the following:

$whereSubClause = array();

/* Firstly identify if we have 'All' as the manufacture - 
 * if we do, we don't need to add constraints to the query.
 */
if ( 'All' != $manufacturer ){

  $mfg = sqlite_escape_string($manufacturer);
  $whereSubclause[] = "`Manufacturer` = {$mfg}";

}

if ( 'Any' != $model ){

    $model = sqlite_escape_string($model);
    $whereSubclause[] = "`Model` = {$model}";

}

if ( 'Any' != $forsale ){

    $forSale = sqlite_escape_string($forsale);
    $whereSubclause[] = "`ForSale` = {$forSale}";

}

// We can now safely construct the SQL -- 
// I haven't provided the method for this, but you'd end up with:

$query = "SELECT * FROM `vehicles` WHERE (`Manufacturer` = 'AMG') AND (`Manufacturer` = 'AMG') AND (`ForSale` = '1')"

I would then personally output the data into an array, which can then be passed to a view. Using a foreach loop, I would construct the rows in the table.

\$\endgroup\$
3
  • \$\begingroup\$ Very helpful! Lots of good basic PHP fundamentals. I just learned some about templating which I'm assuming is considered one approach to MVC. Are there other approaches that are good for separating data and presentation logic? \$\endgroup\$
    – Chris
    Jul 2, 2013 at 23:49
  • \$\begingroup\$ MVC is basically a pattern or architecture; pretty much all architectures support the separation of concerns. There's MVP MVVM and PAC. \$\endgroup\$
    – bear
    Jul 3, 2013 at 17:47
  • \$\begingroup\$ Makes sense. Thanks again! I'll upvote your answer as soon as I have enough rep points to do so! \$\endgroup\$
    – Chris
    Jul 3, 2013 at 18:27
3
\$\begingroup\$

With regard to the PHP injection issue, I actually think parameterisation of the inputs is a better method.

Something like the example below (sorry it is not structured like your specific case):

$sql = "SELECT articleId, title, html, tldr, createdDate, lastUpdate FROM Articles WHERE articlePK=? and status='published'";
$stmt = $con->prepare($sql);
$stmt->bind_param("i",$articlePK);
$stmt->execute();
$stmt->bind_result($articleId, $title, $html, $tldr, $date, $lastUpdate);
$stmt->fetch();  //only ever return one row 
$stmt->close();

Hope that helps.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.