You don't give a clear description of the erroneous behavior. Is the entire page blank? Is just the response section empty? Is there a table with no rows? Do you get rows containing only player names and IDs? What's the URL for a live page, so that we may see for ourselves?
Your script is vulnerable to SQL injection via the 'id' parameter. Also, you should refactor the repeated code (the two table row generating loops and the start of table strings) into functions. A for loop is clearer than a while loop in this instance. Instead of the regexp, you can use is_numeric. Try the [php] tags when you're posting PHP code; the result is much easier to read and can help you catch some simple syntactical mistakes.
Here's the first few iterations of refactoring (untested):
PHP Code:
<?php
//db.php
$link = new PDO("mysql:host=localhost;dbname=liguehs_league", "...", "...");
$link->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
// trade.php
/* $statFields holds the fields from the players table that we're interested in.
Keys are the column names, values are labels.
*/
$statFields = array(
'name' => 'Nom',
'contract_length' => 'Contrat',
'salary' => 'Salaire',
'age' => 'Age',
'off' => 'Off', 'def' => 'Def', 'oa' => 'Oa'
);
$getTrades = $link->prepare('SELECT team1,players1,team2,players2
FROM `trade` WHERE `id`=?');
$getStats = $link->prepare('SELECT `' . implode('`,`', array_keys($statFields)) . '`
FROM `players` WHERE `name`=?');
// getStatRows() and playerTable() should probably be defined in a separate file
function getStatRows($playersString) {
global $getStats,$statFields;
static $fieldCnt = count($statFields);
$players = explode(",",rawurldecode($playersString));
$count=count($players);
$rows=array();
for($i=0;$i < $count ;++$i) {
if (is_numeric($players[$i]) ) {
$rows[] = "<tr><td colspan='$fieldCnt'>$players[$i]</td></tr>\n";
} else {
$stats = $getStats->execute(array($players[$i]))->fetch();
$rows[] = "<tr><td>" . implode('</td><td>', $stats). "</td></tr>\n";
}
}
return implode('', $rows);
}
function playerTable($team, $players) {
global $statFields;
static $hdr = '<th>' . implode('</th><th>', $statFields) . '</th>';
static $fieldCnt = count($statFields);
$start = "<table>
<thead>
<tr><th colspan='$fieldCnt'>$team</th></tr>
<tr>$hdr</tr>
</thead>
<tfoot>
<tr><th colspan='$fieldCnt'> </th></tr>
</tfoot>
";
return $start . getStatRows($players) . '</table>';
}
// now that everything's set up, do the work
try {
$trade = $getTrades->execute(array($_GET['id']))->fetch();
$response = playerTable($trade['team1'], $trade['players1']) . '<br/>'
. playerTable($trade['team2'], $trade['players2']);
} catch (PDOException $exc) {
$response = "Internal error. It's been logged and we'll look into it.";
error_log((string)$exc);
}
?>
You could further refactor to produce a grid view class that you can then use to display any query result in a table, and a data access layer that handles all of the SQL queries for you.
Note that the above implementation of getStatRows runs a separate query for each named player, which is inefficient. Here's an alternate implementation that runs a single query for each team, but sacrifices the clarity and security of prepared queries and doesn't preserve the order of players.
PHP Code:
function formatRow($row) {
return "<tr><td>" . implode('</td><td>', $row) . "</td></tr>\n";
}
function getStatRows($playersString) {
global $link,$statFields;
static $fieldCnt = count($statFields);
static $getStats = 'SELECT `' . implode('`,`', array_keys($statFields)) . "` FROM `players` WHERE `name` IN ('";
$nums = array();
$names = array();
$rows = array();
$players = explode(",",rawurldecode($playersString));
foreach ($players as $player) {
if (is_numeric($player)) {
$rows[] = "<tr><td colspan='$fieldCnt'>$players[$i]</td></tr>\n";
} else {
$names[] = $player;
}
}
$stats = $link->query($getStats . implode("','", $names) . "')")->fetchAll(PDO::FETCH_NUM);
return implode('', array_merge(array_map('formatRow', $stats), $rows));
}
LinkBack URL
About LinkBacks
Reply With Quote
