Script For Review

[son]

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

[justsomeguy]

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.

[son]

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

[justsomeguy]

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.

[son]

Many thanks...Son