Re: Code Review PLEASE !!!

* Thus wrote Matthew Oatham (matthewoatham@hotmail.com): > Hi, > > I am a newbie PHP programmer, I have some code that works but I want some tips on how I an > Improve my code, i.e. should I be doing my updates / deletes on same php page as the display page, > am I using transactions correctly, am I capturing SQL errors correctly am I handling form data as > efficient as possible? > > > <? use full php tags: <?php > if (isset($_POST['delete'])) { > $deleteList = join(', ', $_POST['delete']); be careful here! if I add to the _POST data something like &delete%5B%5D=%29%20OR%281 then I just deleted all your records. > //Enter info into the database > mysql_query("begin"); > foreach ($_POST['fleet_id'] as $key => $value) { > $fleetCode = $_POST['fleet_code'][$key]; > $historyUrl = $_POST['history_url'][$key]; > $downloadUrl = $_POST['download_url'][$key]; I would set up your array's so they are a little more readable and dont rely on autoincrementing array key's. So your loop would look something like this: foreach($_POST['fleets'] as $fleet_id => $fleet) { echo $fleet_id; // the fleet_id echo $fleet['code']; echo $fleet['urls']['history']; echo $fleet['ulrs']['download']; ... > mysql_query("UPDATE imp_fleet SET fleet_code = '$fleetCode', history_url = > '$historyUrl', download_url = '$downloadUrl' WHERE fleet_id = $value") or > die (mysql_error()); escape your data that is untrusted, see http://php.net/mysql_real_escape_string > } > if ($deleteList) { > mysql_query("DELETE FROM imp_fleet WHERE fleet_id IN($deleteList)") or die > (mysql_error()); > } > if (mysql_error()) { this is never reached if there was an error since you die'd() on errors befor. > } > } > ... > > while ($row = mysql_fetch_array($sql)) { > ?> > <tr> > <td><input type="text" name="fleet_code[]" > value="<?=$row['fleet_code']?>"><input type="hidden" > name="fleet_id[]" value="<?=$row['fleet_id']?>"></td> > <td><input type="text" name="download_url[]" > value="<?=$row['download_url']?>"></td> > <td><input type="text" name="history_url[]" > value="<?=$row['history_url']?>"></td> > <td><input type="checkbox" name="delete[]" > value="<?=$row['fleet_id']?>"></td> // set up a variable for the input form names for easier reading. $var_name = 'fleets['.$row['fleet_id'].']'; ?> <td> <input name="<?php echo $var_name?>[code]"> <input name="<?php echo $var_name?>[urls][history]"> <input name="<?php echo $var_name?>[urls][download]"> ... I'd suggest not using the <?= shorthand also. HTH, Curt -- "I used to think I was indecisive, but now I'm not so sure."