Seperate the concerns
The first problem I see with the current implementation, is that it "magically" acts on the $_SESSION
variable. By doing so, you limit your ability to modify the logic in the future since the implentation of the logic is so tightly soupled with how the data is being stored. You also run the risk of forgetting that you had a function somewhere else in the code that also altered the value of $_SESSION
, thus making the behavior of removeProductFromBasket
unpredictable and vulnerable to bugs.
To fix this issue, you need to decouple your function from the $_SESSION
. You can achieve this by adding a parameter to the function signature that takes in the shopping cart as an array.
function removeProductFromBasket($cart, $itemID)
{
foreach($cart as $key => $productArr) {
if ($productArr['productid'] == $itemID) {
echo $itemID;
$cart[$key]['quantity'] = 0;
}
}
return cart;
}
By modifying the code in this way allows you to handle the $_SESSION
elsewhere, maybe in a wrapper object or something of the sort without affecting the logic of the item removal of the shopping cart. It also allows you to more easily change how the information is persisted. Say you wanted to use Redis or some other data store, you would have less lines to modify to make the change.
Calling the function
The other problem I see with your current implementation is the way the function is called. From your sample code, I'm guessing that the full line ressembles something like this:
echo
Since PHP is executed on the server side, what this is doing is calling the function removeProductFromBasket
when the page gets loaded. This means that by the time the page is rendered and loaded on the client side, the item has already been removed, and the href will look like href="12345"
du to the echo
in your function.
Instead, you should be echoing a valid url to the item removal logic, with Item ID proerly concatenated as a parameter.
form.php
foreach($_SESSION['shoppingCart'] as $key => $item) {
echo '<a href="http://www.example.com/path/to/file.php?itemId='.$item['productid'].'">Remove this</a>
}
item_removal.php
<?php
function removeProductFromBasket($cart, $itemID)
{
foreach($cart as $key => $productArr) {
if ($productArr['productid'] == $itemID) {
$cart[$key]['quantity'] = 0;
}
}
return cart;
}
$_SESSION['shoppingcart'] = removeProductFromBasket($_SESSION['shoppingcart'], $_GET['itemId']);
?>
Of course there is still room for improvement, for example by creating an object ShoppingCart
and a wrapper Session
around the $_SESSION
variable, but this should be a good place for you start and build upon.