son Posted June 8, 2009 Share Posted June 8, 2009 I have finished a script where you can add, update and delete colours. The deletion also gets a confirmation message before the colour is ultimately deleted. I would be most grateful if you guys had a look on code and see if my code is up to scratch or if I can streamline any part of it. The code is: echo "\n<h1>Colours</h1>\n";// select modeif (isset($_GET['mode'])) { $mode = $_GET['mode'];} elseif (isset($_POST['mode'])) { $mode = $_POST['mode']; } else { $mode = FALSE; }// select colour idif (isset($_GET['id'])) { $id = $_GET['id'];} elseif (isset($_POST['id'])) { $id = $_POST['id']; } else { $id = FALSE; }// delete a colour after confirmation if (isset($_GET['conf']) && $_GET['conf'] == 'yes') { $del_q = "DELETE FROM colours WHERE colour_id = $id"; if ($del_r = mysqli_query ($dbc, $del_q)) { echo "<p>The colour has been deleted!<br />"; echo "<span class=\"small\"><a href=\"colours.php\"><< Go back</a></span></p>"; include('inc/footer.php'); exit; } }if (isset($_POST['submitted'])) { if ($mode == 'new') { if (!isset($_POST['colour']) OR empty($_POST['colour'])) { $colour = FALSE; } else { $colour = escape_data($_POST['colour']); $colour = str_replace($issues, $replacements, $colour); } } else { $colour = escape_data($_POST['colour']); $colour = str_replace($issues, $replacements, $colour); } if ($colour) { if ($mode == 'new') { $new_q = "INSERT INTO colours (colour) VALUES ('$colour')"; if ($new_r = mysqli_query ($dbc, $new_q)) { echo "<p>The colour has been added!<br />"; echo "<span class=\"small\"><a href=\"colours.php\"><< Go back</a></span></p>"; include('inc/footer.php'); exit; } } elseif ($mode == 'del') { if (!isset($_GET['conf']) && $_GET['conf'] != 'yes') { echo "<p>Are you sure you want to delete the colour?<br />"; echo "<span class=\"small\"><a href=\"colours.php?mode=del&conf=yes&id=" . $id . "\"><strong>Delete >></strong></a></span></p>"; include('inc/footer.php'); exit; } } elseif ($mode == 'upd') { $upd_q = "UPDATE colours SET colour = '$colour' WHERE colour_id = $id"; if ($upd_r = mysqli_query ($dbc, $upd_q)) { echo "<p>The colour has been updated!<br />"; echo "<span class=\"small\"><a href=\"colours.php\"><< Go back</a></span></p>\n"; include('inc/footer.php'); exit; } } else { echo "<p>Your submission could not be processed due to a system error.</p>\n"; } } else { echo "<p>Some of your entries need to be amended:<br />"; if ($mode == 'upd') { echo "<strong>Please enter a new colour when updating an existing colour.</strong></p>\n"; } else { echo "<strong>Please enter the name of the colour you wish to add.</strong></p>\n"; } }} else { echo "<p>In this section you can add, update and delete product colours.<br />All fields marked with * are required to add a colour.</p>\n";}?><h2>Add a new colour</h2><form action="colours.php" method="post"><p><label for="colour">Colour (20 characters)*</label> <input type="text" name="colour" id="colour" size="25" maxlength="20" /> <input type="hidden" name="submitted" value="TRUE" /><input type="hidden" name="mode" value="new" /><input type="submit" name="submit" value="Add colour" /></p></form><?php$q = 'SELECT colour_id, colour FROM colours ORDER BY colour';$r = mysqli_query($dbc, $q);if (mysqli_num_rows($r) > 0) {echo "<h2>Update and delete colours</h2>\n";while (list($colour_id, $colour) = mysqli_fetch_array($r, MYSQLI_NUM)) {echo "<h3><label for=\"colour\">$colour</label></h3>\n";echo "<form action=\"colours.php\" method=\"post\" style=\"float:left; margin-right:5px;\">\n<input type=\"hidden\" name=\"id\" value=\"$colour_id\" /> <input type=\"text\" name=\"colour\" id=\"colour\" size=\"25\" maxlength=\"20\" > <input type=\"hidden\" name=\"submitted\" value=\"TRUE\" /><input type=\"hidden\" name=\"mode\" value=\"upd\" /><input type=\"submit\" name=\"submit\" value=\"Update colour\" />\n</form>\n";echo "<form action=\"colours.php\" method=\"post\">\n<input type=\"hidden\" name=\"id\" value=\"$colour_id\" /><input type=\"hidden\" name=\"colour\" id=\"colour\" value=\"$colour\"> <input type=\"hidden\" name=\"submitted\" value=\"TRUE\" /><input type=\"hidden\" name=\"mode\" value=\"del\" /><input type=\"submit\" name=\"submit\" value=\"Delete colour\" />\n</form><br clear=\"left\" />\n"; } } Thank you,Son Link to comment Share on other sites More sharing options...
justsomeguy Posted June 8, 2009 Share Posted June 8, 2009 This line should use or instead of and:if (!isset($_GET['conf']) && $_GET['conf'] != 'yes') {Other than that, I would just say it needs to be formatted a little better to make it easier to read, but it's not bad. Link to comment Share on other sites More sharing options...
son Posted June 8, 2009 Author Share Posted June 8, 2009 This line should use or instead of and:if (!isset($_GET['conf']) && $_GET['conf'] != 'yes') {Other than that, I would just say it needs to be formatted a little better to make it easier to read, but it's not bad.Thanks for your input. With formatting do you mean the way I use tab, line breaks and comments? Or is there anything else I can improve on. Really appreciate to get an opinion on best practice...Son Link to comment Share on other sites More sharing options...
justsomeguy Posted June 8, 2009 Share Posted June 8, 2009 Just tabs mostly, there are several things in the same column that are confusing to look at. With this block, for example: if (isset($_POST['submitted'])) { if ($mode == 'new') { if (!isset($_POST['colour']) OR empty($_POST['colour'])) { $colour = FALSE; } else { $colour = escape_data($_POST['colour']); $colour = str_replace($issues, $replacements, $colour); } } else { It's not immediately apparent which block the brackets before the last else are closing. This is easier to read: if (isset($_POST['submitted'])) { if ($mode == 'new') { if (!isset($_POST['colour']) OR empty($_POST['colour'])) { $colour = FALSE; } else { $colour = escape_data($_POST['colour']); $colour = str_replace($issues, $replacements, $colour); } } else { A lot of it is just personal preference though. Link to comment Share on other sites More sharing options...
son Posted June 8, 2009 Author Share Posted June 8, 2009 Many thanks...Son Link to comment Share on other sites More sharing options...
Recommended Posts
Archived
This topic is now archived and is closed to further replies.