The cart boss! The CART!!

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
trent2800
Forum Commoner
Posts: 48
Joined: Mon Oct 02, 2006 7:02 am

The cart boss! The CART!!

Post by trent2800 »

I used this tutorial as a basis for my cart: http://www.thewatchmakerproject.com/jou ... pping-cart

It used an 'exploding strings' method which I found to be clunky at best. So, I changed it around to use an array in the session variable. I think this is superior but, I submit my code to you, the ultimate code gurus to pick apart... be gentle... but not too gentle... I'll never learn that way.

It uses two files: functions.php and cart.php.

functions.php:

Code: Select all

<?php
function writeShoppingCart() {
	$cart = $_SESSION['cart'];
	if (!$cart) {
		return '<center><b>0</b> items</center>';
	} else {
		$s = (count($items) > 1) ? 's':'';
		return '<p>You have <a href="cart.php">'.count($cart).' item'.$s.' in your shopping cart</a></p>';
	}
}

function showCart() {
	$cart = $_SESSION['cart'];
	if ($cart) {
		$output[] = '<form action="cart.php?action=update" method="post" id="cart">';
		$output[] = '<table>';
		foreach ($cart as $id=>$qty) {
			$Category = substr($id, 0, 1);
			$ProductID = substr_replace($id,'',0,1);
		
			$dbh=mysql_connect ("localhost", "***", "***") or die ('I cannot connect to the database, try again later.');

			mysql_select_db ('guttersg_ggoods');
			
			switch ($Category) {
				case 'c':
					$query = "SELECT * FROM CLOTHING WHERE ProductID = '".$ProductID."'";
					break;
				case 'i':
					$query = "SELECT * FROM INCENSE WHERE ProductID = '".$ProductID."'";
					break;
			}
			
			$result = mysql_query($query) or die('Query failed.');
			
			$row = mysql_fetch_assoc($result);
			
			$output[] = '<tr>';
			$output[] = '<td><a href="cart.php?action=delete&id='.$id.'" class="r">Remove</a></td>';
			$output[] = '<td>'.$row['Name'].'</td>';
			$output[] = '<td>$'.$row['Price'].'</td>';
			$output[] = '<td><input type="text" name="qty'.$id.'" value="'.$qty.'" size="3" maxlength="3" /></td>';
			$output[] = '<td>$'.($row['Price'] * $qty).'</td>';
			$total += $row['Price'] * $qty;
			$output[] = '</tr>';
		}
		$output[] = '</table>';
		$output[] = '<p>Grand total: <strong>$'.$total.'</strong></p>';
		$output[] = '<div><button type="submit">Update cart</button></div>';
		$output[] = '</form>';
	} else {
		$output[] = '<p>You shopping cart is empty.</p>';
	}
	return join('',$output);
}
?>
cart.php:

Code: Select all

<?php

session_start();

require_once('includes/db.php');

require_once('includes/global.php');

require_once('includes/functions.php');

$cart = $_SESSION['cart'];
$action = $_GET['action'];
$id = $_GET['id'];

switch ($action) {
	case 'add':
		if ($cart) {
			if ($cart[$id]) {
				$cart[$id] += $_POST['quantity'];
			} else {
				$cart[$id] = $_POST['quantity'];
			}
		} else {
			$cart = array();
			$cart[$id] = $_POST['quantity'];
		}
		break;
	case 'delete':
		if ($cart) {
			unset($cart[$id]);
		}
		break;
	case 'update':
	if ($cart) {
		foreach ($_POST as $key=>$value) {
			if (stristr($key,'qty')) {
				$id = str_replace('qty','',$key);
				$cart[$id] = $value;
			}
		}
	}
	break;
}
$_SESSION['cart'] = $cart;
?><!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
	
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
	<title>Gutter's Goods Checkout</title>
	<link rel="stylesheet" href="css/styles.css" />
</head>

<body>

<div id="shoppingcart">

<h1>Your Shopping Cart</h1>

<?php
echo writeShoppingCart();
?>

</div>

<div id="contents">

<h1>Please update quantities and click Submit to continue.</h1>

<?php
echo showCart();
?>

<p><a href="index.php">Continue Shopping</a></p>

</div>

</body>
</html>
So, if you can find it in your collectively infinite wisdom(s) to rip my code a new one, please do.

Thanks,

Tyler P

Jcart | Removed Mysql login credentials
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

It looks fine for a simple, MySQL only script. The only thing I might change on that level is to add some addtional support functions to make your cart.php a little more modular. Hopefully from working on this code you can get a glimpse of how using OOP might improve this code.
(#10850)
dancaragea
Forum Newbie
Posts: 10
Joined: Sat Oct 14, 2006 2:03 pm

Re: The cart boss! The CART!!

Post by dancaragea »

Actually it is bad. Unsecure and ugly code. The only thing that is nice is the formatting of the code. Since this is about code critique let me comment your code a bit:
trent2800 wrote:

Code: Select all

$s = (count($items) > 1) ? 's':'';
Where is this $items coming from? Are you relying on register_globals? DON'T!!! Apart from the fact that support for register_globals has been dropped in newer versions of php, relying on this could lead to big vulnerabilities if used unproperly.
Instead of assuming that you will always have $something for the vars you get from post/get/cookies just do a $something=$_GET['something'];
With the proper sanitization of the input your script will be bulletproof.
trent2800 wrote:

Code: Select all

$output[] = '<form action="cart.php?action=update" method="post" id="cart">';
		$output[] = '<table>';
Why bother to save each line of output to an array just to join the lines in the end? You're just loosing time and memory your way. Just make $output=''; at the begining of the function then instead of $output[] = 'blabla' do $output.= 'blabla'; and simply
return $output;

trent2800 wrote:

Code: Select all

foreach ($cart as $id=>$qty) {
............................
		
			$dbh=mysql_connect ("localhost", "***", "***") or die ('I cannot connect to the database, try again later.');

			mysql_select_db ('guttersg_ggoods');
So let me see: if you have 10 products in your cart you'll be reconnecting 10 times to the database. Why? That mysql_connect and mysql_select_db has nothing to do inside the foreach loop. Actually it shouldn't even be in the function but outside it somewhere, before the function call.
Then what would you do when you will have to use this cart for another customer? Search all your code for all occurences of mysql_connect and change the user/pass in all places? Just create a db_connect() function which takes all params (host, dbname, user, pass) and call that with some constants you define for host/db/user/pass. This way you will just have to change the constants once for each customer.

trent2800 wrote:

Code: Select all

$result = mysql_query($query) or die('Query failed.');
die? Just die? for a shopping cart? Ok, I'm beeing mean here but you could write an error() function to display a sorry page and let the user continue what he was doing (or better yet use set_error_handler() / trigger_error() )
trent2800 wrote:

Code: Select all

$action = $_GET['action'];
$id = $_GET['id'];
NEVER EVER in your entire life do that again!! Never trust user input! If the action or id *should* be an integer then do a $id = intval($_GET['id']);
If they should be strings, at the very least do $id = addslashes($_GET['id']);
You need to check the status of magic_quotes option before you addslashes because with magic_quotes turned on slashes are added automatically and another addslashes would duplicate the slashes.
Even better than addslashes is mysql_real_escape_string().

Well, that's all I could find in a glance. I've given you hints - go search the manual to see what all these functions and options are about and good luck with your programming.

Dan
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

Shows how much I looked at the code. Really good comments dancaragea.
(#10850)
User avatar
Cameri
Forum Commoner
Posts: 87
Joined: Tue Apr 12, 2005 4:12 pm
Location: Santo Domingo, Dominican Republic

Post by Cameri »

I did some testing, a 10 loop ran another 1,000,000 loop with the following cases:

Code: Select all

$n = (int)$some_data;

Code: Select all

$n = intval($some_data);
Where $some_data was a string containing: 420000000000000000000

The results when using intval():

Code: Select all

1.4768209457397
2.4236171245575
1.4313340187073
1.3956470489502
1.4835917949677
1.3982779979706
1.5036118030548
2.1701409816742
1.5360929965973
2.4611489772797
The results when using type casting (int):

Code: Select all

1.1849999427795
0.96212697029114
0.96519899368286
0.97235488891602
1.1018259525299
0.98276495933533
0.95604610443115
0.95767402648926
0.95730113983154
0.96163487434387
It seems that type casting is faster than intval, a function call. Choose whicheve you feel more comfortable with.
Post Reply