basic OOP shoppping cart(very basic)

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

enoc22
Forum Commoner
Posts: 33
Joined: Wed Apr 01, 2009 12:45 pm

basic OOP shoppping cart(very basic)

Post by enoc22 »

Starting to get a grip with php. wrote the following for a very basic shopping cart and wanted to know any ways i could do something simpler,faster, or better.
I'm open to everything.

Heres the code(updated)

Code: Select all

<?php
[color=#0000FF][b]CLASS[/b][/color] store{
    [color=#0000FF][b]public[/b][/color] [color=#FF8000]$cart[/color]=array();
    [color=#0000FF][b]Public[/b][/color] [color=#FF8000]$R[/color];
    [color=#0000FF][b]Public[/b][/color] [color=#FF8000]$X[/color]="apples";
    [color=#0000FF][b]Public[/b][/color] [color=#FF8000]$T[/color]="oranges";
    [color=#0000FF][b]Function[/b][/color] __construct([color=#FF8000]$name[/color]){
        echo "<h1 align='center'>Welcome to [color=#FF8000]$name[/color]</h1>";
    }
    [color=#0000FF][b]Function[/b][/color] BUY([color=#FF8000]$count[/color], [color=#FF8000]$item[/color]){
        SWITCH([color=#FF8000]$item[/color]){
            CASE "oranges":
                $this->cart[]="[color=#FF8000]$count $this->T[/color]";
                BREAK;
            CASE "apples":
                [color=#FF8000]$this->cart[][/color]=("[color=#FF8000]$count $this->X[/color]");
                BREAK;
            DEFAULT:
                [color=#FF8000]$this->cart[][/color]=(" $item ");
        }
    }
}
[color=#0000FF][b]CLASS[/b][/color] cart [color=#0000FF][b]EXTENDS[/b][/color] store{
    Function __construct(){
    }
 
    [color=#0000FF][b]Protected [/b][/color]Function empty_cart(){
        [color=#0000FF][b]Function[/b][/color] find_not([color=#FF8000]$var[/color]){
            [color=#0000FF][b]IF[/b][/color](!(stristr([color=#FF8000]$var[/color], "apples")) [color=#0000FF][b]AND[/b][/color] !(stristr([color=#FF8000]$var[/color], "oranges"))){
                return [color=#FF8000]$var[/color];
            }
        }   
        [color=#FF8000]$r[/color]=array_filter([color=#FF8000]$this->cart[/color], "find_not");
        [color=#0000FF][b]IF[/b][/color](!empty([color=#FF8000]$r[/color])){
            echo"<h2> Sorry We <u>do not</u> have the folowing items in stock</h2>";
            echo"<blockquote>";
            [color=#0000FF][b]FOREACH[/b][/color]([color=#FF8000]$r[/color] [color=#0000FF][b]AS[/b] [/color][color=#FF8000]$A[/color]){
                [color=#FF8000]$ill[/color]=strstr([color=#FF8000]$A[/color], " ");
                echo "--[color=#FF8000]$ill[/color]<br>";
                    
            }
            echo "</blockquote>";
        }
    }
            
    [color=#0000FF][b]Public Function[/b][/color] view(){
        echo"<h3><font color='blue'>This Is your Cart</font></h3>";
 
        [color=#0000FF][b]Function[/b][/color] find_oranges([color=#FF8000]$var[/color]){
            Return(stristr([color=#FF8000]$var[/color], "oranges"));
        }
        [color=#0000FF][b]Function[/b][/color] find_apples([color=#FF8000]$var[/color]){
            Return(stristr([color=#FF8000]$var[/color], "apples"));
        }
        [color=#FF8000]$apples[/color]=array_filter([color=#FF8000]$this->cart[/color], "find_apples");
        [color=#FF8000]$oranges[/color]=array_filter([color=#FF8000]$this->cart[/color], "find_oranges");
        [color=#FF8000]$r[/color]=array_filter([color=#FF8000]$this->cart[/color], "find_not");
        echo"you have " . array_sum([color=#FF8000]$oranges[/color]) . " [color=#FF8000]$this->T[/color] in your cart<br>";
        echo"you have " . array_sum([color=#FF8000]$apples[/color]) . " [color=#FF8000]$this->X[/color] in your cart<br>";
        $this->empty_cart();
    }       
   
}
[color=#FF8000]$freds_corner_store[/color]=new store("Freds Corner store");[color=#00BF00]//adds the store name on top of page[/color]
[color=#00BF40]/*------------------New Cart------------------------------------*/[/color]
[color=#FF8000]$my_cart[/color]= new cart();
[color=#FF8000]$my_cart->BUY[/color](22, "oranges");
[color=#FF8000]$my_cart->BUY[/color](62, "oranges");
[color=#FF8000]$my_cart->BUY[/color](5, "bannanas");
[color=#FF8000]$my_cart->BUY[/color](9, "apples");
[color=#FF8000]$my_cart->BUY[/color](2, "pears");
[color=#FF8000]$my_cart->BUY[/color](100, "apples"); //
[color=#FF8000]$my_cart->view[/color]();
 
 
?>
 
Thanks a Million

Oliver
max529
Forum Commoner
Posts: 50
Joined: Sat May 19, 2007 4:10 am

Re: basic OOP shoppping cart(very basic)

Post by max529 »

hi,

why are you doing ...

Code: Select all

..
   Function BUY($count, $item){
         SWITCH($item){
            CASE "oranges":
                 $this->cart[]="$count $this->T";
                 BREAK;
             CASE "apples":
...
 

Code: Select all

when you can do 
 
   Function BUY($count, $item){      
                $this->cart[$item]= $this->cart[$item]+$count;
....
 
and you should be storing the cart items in some session variables or in database.Storing them in an array wont make them retain the values in the subsequent page request.

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

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

[quote="max529"]when you can do

Code: Select all

 
 
   Function BUY($count, $item){      
                $this->cart[$item]= $this->cart[$item]+$count;
....
 
When you can do:

Code: Select all

  function buy($count, $item){      
                $this->cart[$item] += $count;
But there are many other problems, the smallest is you capitalization is very un-PHP. The bigger problem that your classes are inverted -- store should really inherit cart and neither should have hardcoded values in them.
(#10850)
User avatar
php_east
Forum Contributor
Posts: 453
Joined: Sun Feb 22, 2009 1:31 pm
Location: Far Far East.

Re: basic OOP shoppping cart(very basic)

Post by php_east »

arborint wrote:-- store should really inherit cart.
now that does sound a bit odd to me. because it does look like that's what's been done.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

php_east wrote:
arborint wrote:-- store should really inherit cart.
now that does sound a bit odd to me. because it does look like that's what's been done.

Code: Select all

CLASS cart EXTENDS store{
(#10850)
enoc22
Forum Commoner
Posts: 33
Joined: Wed Apr 01, 2009 12:45 pm

Re: basic OOP shoppping cart(very basic)

Post by enoc22 »

Thank you all for your input so far.. it really is a great help me!!

@max529 thanks for input about the Buy funtion. i will try it. as far as storing the cart in a session. i did think of that.. i just haven't put it in yet. i want to get the base finished before i move on.

@arborint Thank you as well for your input
Could you explaine->
the smallest is your capitalization is very un-PHP
What would be the proper way to capitalize in PHP?

i'm a bit confused with the following....
arborint wrote:
php_east wrote:
arborint wrote:-- store should really inherit cart.
now that does sound a bit odd to me. because it does look like that's what's been done.

Code: Select all

CLASS cart EXTENDS store{
Is this what it should be?

Code: Select all

[b][color=#000000]CLASS[/color][/b] [color=#FF0000]store[/color] [b][color=#000000]EXTENDS[/color][/b] [color=#FF0000]cart{[/color]
if it's it's not too much to ask.. could you also explain why it should be the other way round?

Again i appreciate all your input and suggestions


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

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

enoc22 wrote: What would be the proper way to capitalize in PHP?
Look at something like the Zend Framework style manual to see how PHP should be formatted. That is probably the standard at this point.
i'm a bit confused with the following....
enoc22 wrote: if it's it's not too much to ask.. could you also explain why it should be the other way round?
A cart is like a cash register. Many stores may use the same kinds of cash register even though the sell different kinds of things. I think of a cart as something that mainly holds SKUs and quantities and knows how to subtotal/grand total.

A cart may also have additional features. Such as also holding qualities of a SKU like color or size (if SKU itself does not specify that). And a cart may also know how to add things like tax and shipping into the grand total. There are plenty of other possible features, but you should stick to the basics -- 1) the ability to add items to the cart, 2) the abilities to get items from the cart to display them, 3) the ability to modify/delete items in the cart.
(#10850)
User avatar
Apollo
Forum Regular
Posts: 794
Joined: Wed Apr 30, 2008 2:34 am

Re: basic OOP shoppping cart(very basic)

Post by Apollo »

enoc22, you're kinda "abusing" the OOP concept. Not everything has to inherit from eachother all the time. "class X extends Y" means that X is a special kind of Y, but this relationship does not hold with stores and carts. A store has a cart, or a cart belongs to a store, or uses a store, but they're not a similar kind of object. There isn't a single situation where you want to apply the same functionality on both (i.e. function(x) where x can be either a store or cart).

Furthermore, try to stick with an important principle: no redundancies. You are currently selling apples and oranges, and you have literally spelled these out at several places. Not good. Imagine you'd want to add another product, for example strawberries. Ideally there should be ONE place (some list, an array, or database) where you add the string "strawberries", no more.
Why: (amongst many other reasons)
  • Easier to maintain, now you have to edit several places when adding or removing a product (and have to watch out you don't forget one)
  • No risk of typos, if you have the string "apples" in your code 10 times but somewhere you accidentally spelled it "aples", you will get all kinds of headache-inflicting issues (why do oranges work and apples don't!? why can I add apples, but not remove them!? etc)
enoc22
Forum Commoner
Posts: 33
Joined: Wed Apr 01, 2009 12:45 pm

Re: basic OOP shoppping cart(very basic)

Post by enoc22 »

@arborint I just took a look at the Zend framework manual and i think i get your point on capitalization. i will do better on that now.
arborint wrote:i'm a bit confused with the following....
enoc22 wrote: if it's it's not too much to ask.. could you also explain why it should be the other way round?
I was refering to what you had mentioned earlier
arborint wrote:The bigger problem that your classes are inverted -- store should really inherit cart and neither should have hardcoded values in them.
@ApolloThanks for your input
i want to make sure i'm getting the following right..
Apollo wrote:enoc22, you're kinda "abusing" the OOP concept. Not everything has to inherit from eachother all the time. "class X extends Y" means that X is a special kind of Y, but this relationship does not hold with stores and carts. A store has a cart, or a cart belongs to a store, or uses a store, but they're not a similar kind of object. There isn't a single situation where you want to apply the same functionality on both (i.e. function(x) where x can be either a store or cart).
so at least with my shopping cart and store. they should be two seperate classes..?

thank you especialy for the following
arborint wrote:Furthermore, try to stick with an important principle: no redundancies. You are currently selling apples and oranges, and you have literally spelled these out at several places. Not good. Imagine you'd want to add another product, for example strawberries. Ideally there should be ONE place (some list, an array, or database) where you add the string "strawberries", no more.
Why: (amongst many other reasons)

* Easier to maintain, now you have to edit several places when adding or removing a product (and have to watch out you don't forget one)
* No risk of typos, if you have the string "apples" in your code 10 times but somewhere you accidentally spelled it "aples", you will get all kinds of headache-inflicting issues (why do oranges work and apples don't!? why can I add apples, but not remove them!? etc)
I hadn't thought of that at all.....super!!!

I will start corecting my code and post and updated version..... any Other suggestions are welcome


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

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

You may want to walk through implementing your class TDD style here in this thread so you can get input at each step about the possible design decisions that can be made and what best practices might be.
(#10850)
User avatar
php_east
Forum Contributor
Posts: 453
Joined: Sun Feb 22, 2009 1:31 pm
Location: Far Far East.

Re: basic OOP shoppping cart(very basic)

Post by php_east »

arborint wrote:
php_east wrote:
arborint wrote:-- store should really inherit cart.
now that does sound a bit odd to me. because it does look like that's what's been done.

Code: Select all

CLASS cart EXTENDS store{
well, i think it is philosophical. a glass of half filled with water can be viewed as half full or half empty. and as usual with philosophy, there isn't any right or wrong way. it is kinda like when PHP says you can use ' or " but please be consistent.

i could use
class cart extends store
class items extends cart
class transaction extends store

and then have
cart -> new cart
cart->add_item
transaction->new transaction

if (transaction) is approved
store ->do transaction
{
calculate cart total
}

so, perhaps not so important, but surely OOP is very flexible.
enoc22
Forum Commoner
Posts: 33
Joined: Wed Apr 01, 2009 12:45 pm

Re: basic OOP shoppping cart(very basic)

Post by enoc22 »

arborint wrote:You may want to walk through implementing your class TDD style here in this thread so you can get input at each step about the possible design decisions that can be made and what best practices might be.
How would i do that?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

php_east wrote:

Code: Select all

CLASS cart EXTENDS store{
well, i think it is philosophical. a glass of half filled with water can be viewed as half full or half empty. and as usual with philosophy, there isn't any right or wrong way. it is kinda like when PHP says you can use ' or " but please be consistent.[/quote]extends is not like or in the least, and or and || have different precedence so there are small reasons to use one or the other. No matter how much water is in a glass, these will operate the same in PHP.

The important (right or wrong) point I was making is that generally stores contain carts, carts don't contain stores. A store is something with a specific set of SKUs. A cart can hold SKUs from many kinds of stores. If you look at the initial design it is conceptually incorrect. And that is not even getting to Apollo's excellent points about better ways to structure this code.
(#10850)
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Re: basic OOP shoppping cart(very basic)

Post by Christopher »

enoc22 wrote:
arborint wrote:You may want to walk through implementing your class TDD style here in this thread so you can get input at each step about the possible design decisions that can be made and what best practices might be.
How would i do that?
Start with the first, most important thing that the cart needs to do. Implement that. If you were really doing TDD you would write the test for that -- then implement code to pass the test. Then go on to the next most important thing. The process has a number of positive effects on the design process.

So forget your code above ... what it the one, first, most important thing (to your clients or you) that your cart should do?
(#10850)
enoc22
Forum Commoner
Posts: 33
Joined: Wed Apr 01, 2009 12:45 pm

Re: basic OOP shoppping cart(very basic)

Post by enoc22 »

Ahah...I see now (sorry I'm sometimes a bit slow on the draw).
This actually isn't for any client/commercial...more of just learning php.but you are right in that i need some point to focus on accomplishing...
For this project it would be to do the following in the most efficient way
  • ●being able to add product to the shopping cart(some product multiple times)
  • ●have it display the total of each product in the cart.
  • ●display and out of stock error for product not in the "store"

Thanks
Oliver
Post Reply