1
\$\begingroup\$

I'm fairly new to PHP and am following e-commerce tutorials. When exploring a way to resolve a problem I had with a PHP code block, I came upon a post another person had with the same code block. The post suggested a way to simplify the code.

Original code block:

<?php
if (isset($_POST ['pid'])) {
    $pid = $_POST ['pid'];
    $wasFound = false;
    $i = 0; 
    if (!isset ($_SESSION["cart_array"]) || count($_SESSION ["cart_array"])<1){
        $_SESSION["cart_array"] = array (0=> array("item_id"=> $pid, "quantity" => 1));
    }  else {
        foreach ($_SESSION["cart_array"] as $each_item) {
            $i++; 
            while(list($key,$value)=each($each_item)){
            if ($key== "item_id" && $value == $pid) {
                array_splice ($_SESSION["cart_array"], $i-1,1, array(array("item_id"=>$pid,"quantity"=> $each_item['quantity']+1)));
                $wasFound = true;
            }
        }
    }
    if($wasFound==false) {
        array_push ($_SESSION["cart_array"], array("item_id"=> $pid, "quantity" => 1));    
        }
    }
    header("Location: cart.php");
    exit();
}
?> 

This was the suggested simplified code block and an explanation of what to do after changing code. I am unsure how to implement the the part suggested after the code block.

if (isset($_POST['pid'])) {
    // add (+1) item to cart
    $pid = (int)$_POST['pid']; // cast as integer
    // valid pids are > 0
    if($pid > 0){
    if(!isset($_SESSION['cart_array'][$pid])){
    // item is not in the cart, add it with quantity = 1
    $_SESSION['cart_array'][$pid] = array("item_id" => $pid, "quantity" => 1); // I left the array in the cart the same, but it
could also be simplified so that it is only the quantity, since the
item_id is now the cart array index
    } else {
    // item is in the cart, increment quantity
    $_SESSION['cart_array'][$pid]['quantity']++;
    }
    }
    header("location: cart.php");
    exit();
    }

To get the details for the cart items, you need to run ONE query that gets all of them at the same time (putting a query inside of a loop is a resource killer). For the definition of the cart that I have suggested, you can use array_keys to get all the item id's. You would then implode those into a comma separated list and put them into an IN() comparison in the WHERE clause in a query to get all the matching rows at once.

This is what I am considering adding to the code block:

if  ($result = print_r(array_keys('cart_array',$pid))) {
$comma_seperated = implode("," $result);
// then use use $comma_seperated in query where needed later in annother code block?
}

Am I on the right path?

\$\endgroup\$
  • \$\begingroup\$ There are a few rules to improve any code, but they have to be understood before implemented. A good tool is PHPMD which will give to you a lot of code execution's optimization. Object Calisthenics (PHPMD use some of it) are really good to reduce the code. And, after all, IMHO, Clean Coder (Robert UncleBob Martin) and Refactoring (Kent Beck and Martin Fowler) have a lot of good examples. \$\endgroup\$ – Gabriel Heming Mar 30 '17 at 17:13
  • \$\begingroup\$ Thanks. I'll check the suggestions out. I've been watching tutorials and reading documentation to try to better understand before implemented. Just wanted to know if I was in any way getting closer to accomplishing this. Or start over. \$\endgroup\$ – Learn2Earn Mar 30 '17 at 17:39
  • \$\begingroup\$ The way which you had chosen is ok. But it's more about the N+1 problem, which is good to think about it. If you need some material, I can suggest to you the links above: OODesign; Source Making; But Uncle Bob - Principles of OOD. It's just to start. \$\endgroup\$ – Gabriel Heming Mar 30 '17 at 17:45
1
\$\begingroup\$

You don't do a good job of validating $_POST['pid']. You just assume the value passed will be good (a bad mistake when working with user input).

May I suggest something like:

$filterOptions = array('options' => array('min_range' => 1));
$pid = filter_input(INPUT_POST, 'pid', FILTER_VALIDATE_INT, $filter_options);
if(is_null($pid) || $pid === false) {
     // parameter was either not set (null) or failed filter (false)
     // you should handle this somehow
     exit();
}

The two different code examples are not the same in that the cart array is build differently in each. The first would build a numerically-indexed structure at $_SESSION['cart_array'] like this:

[
    0 => [
        'item_id' => {{pid}},
        'quantity' => {{quantity}}
    ],
    1 => [
        ...
    ],
    ...
]

Whereas the second would build an array index by item id like this:

[
    {{pid}} => [
        'item_id' => {{pid}},
        'quantity' => {{quantity
    ], 
    {{pid2}} => [
        ...
    ],
    ...
]

Did you really intend to change this structure? If so, are there other areas of code that might need to change to accommodate this changed structure?


$result = print_r(array_keys('cart_array',$pid))

This makes no sense. cart_array is not an array suitable for use with array_keys(). Why would you use print_r() (an output function) here at all?

If you want to use the second form of the cart data shown above, this is how you would extract pid values from it:

$pidsInCart = array_keys($_SESSION['cart_array']);

With the numerically-indexed cart array, it might be:

$pidsInCart = array_map(
    function($item) {
        return $item['item_id'];
    },
    $_SESSION['cart_array']
);

I would also would be careful in evaluating the result directly, as you could get unexpected truthy/falsey results there I might use:

$pidsInCart = ...;
if(!empty($pidsInCart)) {
     ...
}

Also, use meaningful variable names. $result here is a good example of a not very meaningful name in this context.


Indent your code properly.

\$\endgroup\$
  • \$\begingroup\$ All of this is just exprimentation for learning purposes. Yes there are other areas that would need to be changed also. They are structured nearly identically. Pointing out the differences in the two array types and the extraction methods helps me a great deal. \$\endgroup\$ – Learn2Earn Mar 30 '17 at 20:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.