My online shop

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

My online shop

Post by impulse() »

Firstly, the shop can be viewed here http://stesbox.co.uk/ws
It's only the books section that's complete at the moment. The admin login in 'UN: admin PW: password'.

Here's the code for the books display page:

Code: Select all

mysql_connect("x", "x", "x");
mysql_select_db("shop");

if (isset($_REQUEST['submitImg'])) {
  
  $boxID = $_REQUEST["idImg"];
  $imgSRC = $_REQUEST["imgSrc"];
 

  switch ($_REQUEST['submitImg']) {
    case "Change Image":
      $query = mysql_query("UPDATE products SET pImg='$imgSRC' WHERE pID='$boxID'");
      echo "You have updated the image location. Please refresh to view changes<br>";
      break;
    default:
      echo "How did this happen?";
      break;
    }
}
 


if (isset($_REQUEST['submitDesc'])) {

  $boxID = $_REQUEST["idDesc"];
  $desc = $_REQUEST["desc"];   
  
  switch ($_REQUEST['submitDesc']) {
    case "Change Description":
      echo "You have updated the book description. Please refresh to view changes";
      $query = mysql_query("UPDATE products SET pDesc='$desc' WHERE pID='$boxID'");
      break;
    default:
      echo "How did this happen?";
      break;
     }
}
 


if (isset($_REQUEST['submitPrice'])) {

  $boxID = $_REQUEST["idPrice"];
  $price = $_REQUEST["price"];  
  
  switch ($_REQUEST['submitPrice']) {
    case "Change Price":
      echo "You have updated the price description. Please refresh to view changes";
      $query = mysql_query("UPDATE products SET pPrice='$price' WHERE pID='$boxID'");
      break;
    default:
      echo "How did this happen?";
      break;
     }
}
 



$queryBooks = mysql_query("SELECT * FROM products ORDER BY pID");
?>
  
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title> Stes Shop  </title>
<meta http-equiv="Content-Type"
content="text/html; charset=iso-8859-1" />

<link rel="stylesheet" type="text/css"
href="http://stesbox.co.uk/ws/2.css" />
<link rel="stylesheet" type="text/css" 
href="http://stesbox.co.uk/ws/3.css" />
</head>
<body> 
<div id="outer">
<div id="hdr" align="center">
<font size="7"><b>Stes Shop</b><font size="3">

<div id="bar">X<span style="padding:5px;font-size:11px;"> </span></div>
<div id="bodyblock" align="right">
<div id="l-col" align="center">   
<h4 align="center">Menu</h4>      
<a href="http://stesbox.co.uk/ws/products/books.php"> Books </a><br />
<a href="http://stesbox.co.uk/ws/products/furniture.php"> Furniture </a><br />
<a href="http://stesbox.co.uk/ws/products/electronics.php"> Electronics </a><br />
<a href="http://stesbox.co.uk/ws/products/clothes.php"> Clothes </a><br />

<?php session_start();
echo $_SESSION['loggedIn'];

?>
<form method="post" action="login.php">
<center><br><br><font size="4"><b>Login</b><font size="3">
<br><br><b>UserName:<br></b>
<center><input type="text" name="username">
<b>Password:<br></b>
<center><input type="password" name="password">
<br><br><center><input type="submit" value="Login">
</form>

</div>
<div id="cont">

<h3 align="center">
<font size="5"> Welcome to my test shop
</h3><font size="3">



<?php
while ($res = mysql_fetch_array($queryBooks)) {

?><table border>
<form>
  <tr>
<center>
<font size="4"><b><?php echo $res['pName']; ?></b></tr>
  <tr>
    <td>
<center><img src="<?php echo $res['pImg'];?>" width="<? echo $res['imgWidth'];?>" height="<?php echo $res['imgHeight']; ?>"
        <br><?php if ($_SESSION['loggedIn'] == 1) {
                                    ?> <table border="1" bgcolor="grey">
                                       <tr>
                                         <td>
                                           <form method="post" action="books.php">
                                           <input type="text" name="imgSrc"><br>  
                                           <input type="hidden" name="idImg" value="<?php echo $res['pID']; ?>">
                                           <center><input type="submit" name="submitImg" value="Change Image"><br><br><br>
                                           </form>
                                         </td>
                                       </tr>  
                                       </table><? } ?> 
                              
    </td>
         
    <td></b><i> 
        <?php echo $res['pDesc']; ?> </i> 
    <?php     if ($_SESSION['loggedIn'] == 1) { ?>
                             <table border="1" bgcolor="grey">
                             <tr>
                               <td>
                                 <form method="post" action="books.php">
                                 <input type="text" name="desc"><br>
                                 <input type="hidden" name="idDesc" value="<?php echo $res['pID']; ?>">
                                 <center><input type="submit" name="submitDesc" value="Change Description"><br><br><br>
                                 </form>
                               </td>
                             </tr>  
                             </table><? } ?>
    </td>
  </tr> <br>
  <tr>
    <td>
      <br><br>Price: <b>$<?php echo $res['pPrice']; ?> </b> 
      <?php                    if ($_SESSION['loggedIn'] == 1) { ?>
                                             <table border="1" bgcolor="grey">
                                             <tr>
                                               <td>
                                                 <form method="post" action="books.php">
                                                 <input type="text" name="price"><br>   
                                                 <input type="hidden" name="idPrice" value="<?php echo $res['pID']; ?>">
                                                 <center><input type="submit" name="submitPrice" value="Change Price"><br><br><brb>
                                                 </form>
                                               </td>
                                             </tr>  
                                             </table><? } ?>
   
   
   
    </td>
    
    <td>
        <form method="post" action="books.php">
        <input type="hidden" name="boxID" value="<?php echo $res['id']; ?>"><br><br>
        <input type="submit" name="add" value="Add to cart">
        Quantity <input type="text" name="quantity" value="0">
    </td>
</form>  
</table><br><br><br><br>
<center><img src="<?php echo $res['pImg'];?>" width="<? echo $res['imgWidth'];?>" height="<?php echo $res['imgHeight']; ?>"
        <br><?php if ($_SESSION['loggedIn'] == 1) {
                                    ?> <table border="1" bgcolor="grey">
                                       <tr>
                                         <td>
                                           <form method="post" action="books.php">
                                           <input type="text" name="imgSrc"><br>  
                                           <input type="hidden" name="idImg" value="<?php echo $res['pID']; ?>">
                                           <center><input type="submit" name="submitImg" value="Change Image"><br><br><br>
                                           </form>
                                         </td>
                                       </tr>  
                                       </table><? } ?> 
                              
    </td>
         
    <td></b><i> 
        <?php echo $res['pDesc']; ?> </i> 
    <?php     if ($_SESSION['loggedIn'] == 1) { ?>
                             <table border="1" bgcolor="grey">
                             <tr>
                               <td>
                                 <form method="post" action="books.php">
                                 <input type="text" name="desc"><br>
                                 <input type="hidden" name="idDesc" value="<?php echo $res['pID']; ?>">
                                 <center><input type="submit" name="submitDesc" value="Change Description"><br><br><br>
                                 </form>
                               </td>
                             </tr>  
                             </table><? } ?>
    </td>
  </tr> <br>
  <tr>
    <td>
      <br><br>Price: <b>$<?php echo $res['pPrice']; ?> </b> 
      <?php                    if ($_SESSION['loggedIn'] == 1) { ?>
                                             <table border="1" bgcolor="grey">
                                             <tr>
                                               <td>
                                                 <form method="post" action="books.php">
                                                 <input type="text" name="price"><br>   
                                                 <input type="hidden" name="idPrice" value="<?php echo $res['pID']; ?>">
                                                 <center><input type="submit" name="submitPrice" value="Change Price"><br><br><brb>
                                                 </form>
                                               </td>
                                             </tr>  
                                             </table><? } ?>
   
   
   
    </td>
    
    <td>
        <form method="post" action="books.php">
        <input type="hidden" name="boxID" value="<?php echo $res['id']; ?>"><br><br>
        <input type="submit" name="add" value="Add to cart">
        Quantity <input type="text" name="quantity" value="0">
    </td>
</form>  
</table><br><br><br><br>

And then for login.php


Code: Select all

mysql_connect("x", "x", "x");
mysql_select_db("shop");

$username = $_REQUEST["username"]; # Gets data from the HTML form
$password = $_REQUEST["password"];
#$_SESSION['loggedIn'] = 0; # This is default 0 so every visitor doesn't have admin privs 



$query = mysql_query("SELECT * FROM users WHERE username='$username'");
$numRows = mysql_numrows($query);
$i = 0;



while ($results = mysql_fetch_array($query)) { # Turns DB results into variables
  $usernameFromDB = $results['username'];
  $passwordFromDB = $results['password'];
  }



$encryptedPassword = md5($password); #Encrypts password




if ($username == $usernameFromDB && $passwordFromDB == $encryptedPassword) {
  echo "Logged in"; # If statement for determing if the user has logged in correctly
    session_start();
    $_SESSION['loggedIn'] = 1; # Give user admin privs
  }



else {
  echo "Sorry, that was an incorrect login";
  $_SESSION['loggedIn'] = 0;
  }

echo $_SESSION['loggedIn'];

?>

<form method="post" action="http://stesbox.co.uk/ws/products/books.php">
  <input type="submit" value="Return to page">
  </form>


<?php




  ?>
Where could I have polished up on this. I know there must be many places due to the length of the code.
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Post by jayshields »

Haven't properly looked at it, but the first thing that jumps out on me is that you're using switch statements with only one case and a default case numerous times; why? Just use an if...else statement. Also, you open and close php tags for no reason at the bottom.

Just glancing at it again, there's numerous things that should be changed. It's not valid XHTML; you have no closing slash on your self-closing HTML entities.
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post by impulse() »

I was meaning to fix that. The original plan was to have

Code: Select all

switch ($_REQUEST['submit']) {
  case "Modify Price":
    //code//
  case "Modify Img Src":
    //code//
  case "Modify Description:
    //code//
}
But due to things going in a totally different direction I would've had to recode the forms part which I wasn't too keen on doing so I left it in its untidy standing. I think I will rewrite the whole lot on another day and tidy everything up, including the display output.

Where do I open and close PHP tags unusefully? Do you mean where I use //HTML CODE// <?php } ?> //HTML CODE// ?

Stephen,
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Post by jayshields »

That switch block you posted was missing a double quote and modify price would execute all three pieces of code, modify img src would execute the bottom two, you missed the break;'s.

The empty php tags are right at the bottom.
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

jayshields wrote:That switch block you posted was missing a double quote and modify price would execute all three pieces of code, modify img src would execute the bottom two, you missed the break;'s.

The empty php tags are right at the bottom.
Come on... even silly old me knew that was just pseudo code ;)

It would be a fairly big re-structure but I think it would be good practise to have a stab at cleaning up the separation between the business logic (all your SQL queries and loops) and the display logic (all that markup).

We can walk through this if you want? You'll find the code much easier to maintain in the long run and it will be much more strucutred.
impulse()
Forum Regular
Posts: 748
Joined: Wed Aug 09, 2006 8:36 am
Location: Staffordshire, UK
Contact:

Post by impulse() »

If you point me in the right direction and not actually code it for me, I'd be gratefull :)
Post Reply