current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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();
?>
share|improve this question
1  
See: xkcd.com/327 –  Dave Jarvis Jul 3 '13 at 0:50
    
Ha! I'll try and remember that :) –  Chris Jul 3 '13 at 16:18
add comment

2 Answers

up vote 2 down vote accepted

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.

share|improve this answer
    
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? –  Chris Jul 2 '13 at 23:49
    
MVC is basically a pattern or architecture; pretty much all architectures support the separation of concerns. There's MVP MVVM and PAC. –  bear Jul 3 '13 at 17:47
    
Makes sense. Thanks again! I'll upvote your answer as soon as I have enough rep points to do so! –  Chris Jul 3 '13 at 18:27
add comment
up vote 2 down vote

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.

share|improve this answer
    
Good suggestion! Thanks! –  Chris Jul 9 '13 at 17:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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