Page 1 of 1

The cart boss! The CART!!

Posted: Fri Oct 13, 2006 1:44 am
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

Posted: Sat Oct 14, 2006 1:02 pm
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.

Re: The cart boss! The CART!!

Posted: Sat Oct 14, 2006 3:54 pm
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

Posted: Sat Oct 14, 2006 8:56 pm
by Christopher
Shows how much I looked at the code. Really good comments dancaragea.

Posted: Sat Oct 14, 2006 9:12 pm
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.