Python OOP shopping cart

Question

I completed the following task:

You need to create the foundations of an e-commerce engine for a B2C (business-to-consumer) retailer. You need to have a class for a customer called User, a class for items in inventory called Item, and a shopping cart class calledCart. Items go in Carts, and Users can have multiple Carts. Also, multiple items can go into Carts, including more than one of any single item.

I am a new Pythoner, and don't have good knowledge of OOP. I feel the code is not good. I welcome recommendations to improve the code.Here's my code:

class Item(object):
    def __init__(self,itemname,itemprice):
        self.__itemname = itemname
        self.__itemprice = itemprice

    def GetItemName(self):
        return self.__itemname

    def GetItemPrice(self):
        return self.__itemprice

    def ChangeItemPrice(self,newprcie):
        self.__itemprice = newprcie

class Cart(dict):      #cart dict format:  {itemname:[price,number]}
    def ShowCart(self):
        return self   

class User(object):
    def __init__(self, name):    
        self.name = name
        self.__cartlist = {}
        self.__cartlist[0] = Cart()

    def AddCart(self):
        self.__cartlist[len(self.__cartlist)] = Cart()

    def GetCart(self, cartindex = 0):
        return self.__cartlist[cartindex]

    def BuyItem(self, item, itemnum, cartindex = 0):
        try:
            self.__cartlist[cartindex][item.GetItemName()][1] += itemnum
        except:
            self.__cartlist[cartindex].update({item.GetItemName():[item.GetItemPrice(),itemnum]})

    def BuyCancle(self, itemname, itemnum, cartindex = 0):
        pass


if __name__ == '__main__': 

    item1 = Item('apple', 7.8)
    item2 = Item('pear', 5)   

    user1 = User('John')

    user1.BuyItem(item1, 5)
    print("user1 cart0 have: %s" % user1.GetCart(0).ShowCart())

    user1.BuyItem(item2, 6)
    print("user1 cart0 have: %s" % user1.GetCart(0).ShowCart())


    user1.AddCart()
    user1.BuyItem(item1, 5, 1)
    print("user1 cart1 have: %s" % user1.GetCart(1).ShowCart())

Answer 1

I have some notes on how you structured this, note that some are a bit Python specific.

Your attribute names should not have the class name in them. It's redundant because you'll always be accessing them from something else, ie. Item.name is simpler than Item.itemname. Also you shouldn't use double underscores to prefix the name. A Pythonic style is to use a single underscore to denote an attribute that should be treated as private and not directly accessed. But very often it's Python style to leave out any concept of setters and getters, instead just access them directly. Like this:

>>> apple = Item('apple', 7.8)
>>> print apple.name
apple
>>> apple.price = 7
>>> print apple.price
7

You could almost not have Item be a class, but your spec seems to require it so I wont suggest you change that.

Also you misspelled newitemprice as newitemprcie. You managed to make it consistent between the two (by copy pasting I assume) but keep an eye on little errors like that. They'll only cause headaches.

Your Cart is a little underdeveloped. All it does is subclass dict and then return itself when ShowCart is called? This is redundant since the Cart itself could be passed to places it's needed.

You mentioned in a comment you want to be able to do Cart['apple'] to get the count of apples, but an easier way to do that is to just have an attribute of cart that's a dictionary. For example:

class Cart:
    def __init__(self, items):
        self.items = items

Cart({"apple": 5, "pear": 7})
Cart.items["apple"]

This is a much clearer way to get at the items without needing to inherent a dictionary into the Cart itself.

Maybe the cart could more directly manage items so you could have useful features like counting how many of each item is there. As it stands, you don't do anything special with a cart that has multiple instances of the same item. Why not keep track of that so you could easier print to the user what they have, rather than expecting them to just read a list of items.

While we're on it. Why not implement a method or two to print the contents of a Cart nicer? Maybe the Cart should be where the user actually purchases things? You need to think about what a cart does, and create those functions.

Why make cartlist a dictionary? You should make it a list instead. You can still do cartlist.remove(cart_to_remove). Also don't name it cartlist especially if it's not a list, carts is better. Types don't need to be in names unless it's hard to tell what type a variable is.

Answer 2

There is no need to create a specialized implementation of a dictionary, as you've done here:

class Cart(dict):      #cart dict format:  {itemname:[price,number]}
    def ShowCart(self):
        return self

This, literally does, nothing. The method ShowCart essentially just returns the dictionary, and again, is useless. The only thing this class accomplishes is making the code harder to read, and harder to maintain.

You also have a few minor PEP8 violations. In case you didn't know, PEP8 is Python's official style guide.

• Functions should be named in snake_case.
• There should be two blank lines between top-level code/functions/classes, not one.

Other than that, there's not too much that I can find wrong with this.

Answer 3

Main

First, before I talk about the OOP parts, I'll look at you main. And say everything that could be classed as 'wrong'.

It's really good that you used if __name__ == '__main__'. It normally is really small, such as a single function call, sometimes with a setup for the framework you are using.

Keep your apostrophes consistent. Either ' or ". The exception to that is when one is inside the other.

You should use str.format, it's better than the old %. And allows for more functionality.

print('user1 cart0 have: {}'.format(user1.GetCart(0).ShowCart()))

---

Classes

First, Python has no 'private' variables. _variable is normally used as private, as developers should trust each other. Whereas __variable uses name mangling. No, it's still not private.

Below shows how both work:

class MyClass(object):
    def __init__(self):
        self.a = 'a'
        self._b = 'b'
        self.__c = 'c'

my_class = MyClass()
print my_class.a
# a
print my_class._b
# b
print my_class._MyClass__c
# c

It's normally recommended to only use _variable, as some people have a large dislike for name mangling.

---

It seems like you come from a Java/getter-setter background. Python can do that, but don't.

Python is happy that it has no privates.

For simple public data attributes, it is best to expose just the attribute name, without complicated accessor/mutator methods.

• PEP8

And so I would say that most of your code for Item in bad. And so would change it to be simpler.

class Item(object):
    def __init__(self, name, price):
        self.name = name
        self.price = price

Now we can just change things with ease, and use operators such as +=.

---

Personally I think Cart is useless at the moment. And I would put BuyItem's logic in here.

---

You should change __cartlist to a list. self.__cartlist = [Cart()].

You should change AddCart to use the new list. self.__cartlist.append(Cart())

Again GetCart, why can't we just have __cartlist public? This will remove GetCart, and will make it more Pythonic.

Now time for BuyCart.

• You yourself didn't use GetCart. Shows how much people would love to use it, if given the option.

• Your try is massive.

• Your except is hard to read, and is to long.

• I think amount is better than itemnum.

And so you should store self.__cartlist[cartindex] in a variable. Then you can use it in both try and except with ease.

cart = self.__cartlist[cartindex]
try:
    cart[item.GetItemName()][1] += itemnum
except:
    cart.update({item.GetItemName():[item.GetItemPrice(),itemnum]})
    
# If you made it more Pythonic

cart = self.cart_list[cart_index]
try:
    cart[item.name][1] += amount
except:
    cart.update({
        item.name: [item.price, amount]
    })

The first one removes a bug, and shows that this should be implemented on Cart instead.
The second half shows how following Python's philosophy just makes it easier to read.

This is how I would implement your code:

class Item(object):
    def __init__(self, name, price):
        self.name = name
        self.price = price

class Cart(dict):
    def add_item(self, item, amount):
        try:
            self[item.name][1] += amount
        except IndexError:
            self.update({
                item.name: [item.price, amount]
            })

class User(object):
    def __init__(self, name):    
        self.name = name
        self.carts = [Cart()]

    def add_cart(self):
        self.carts.append(Cart())
        
    def add_item(self, item, amount, cart_index=0):
        self.carts[cart_index].add_item(item, amount)

def main():
    apple = Item('apple', 7.8)
    
    john = User('John')

    # I would choose `john.add_item(apple, 5, 1)`
    # or `john.carts[0].add_item(apple, 5)`
    # Not both.
    john.add_item(apple, 5)
    print("John's first cart has: {}".format(john.carts[0]))

    john.carts[0].add_item(Item('pear', 5), 6)
    print("John's first cart has: {}".format(john.carts[0]))
    
    john.add_cart()
    john.add_item(apple, 5, 1)
    print("John's second cart has: {}".format(john.carts[1]))

if __name__ == '__main__':
    main()

---

As you can probably guess, Python's OOP is much friendlier. If you have to have getters, setters and non-sense, then you didn't pick the right language.