Jump to content

Script For Review


son
 Share

Recommended Posts

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

Edited by son
Link to comment
Share on other sites

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

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

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
 Share

×
×
  • Create New...