6
\$\begingroup\$

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())
\$\endgroup\$
  • 1
    \$\begingroup\$ Was this brief specifically for Python or did you get to choose your own language? \$\endgroup\$ – SuperBiasedMan Sep 23 '15 at 15:13
  • \$\begingroup\$ @SuperBiasedMan I have chosen Python. \$\endgroup\$ – burce pan Sep 23 '15 at 15:22
  • 1
    \$\begingroup\$ To do this in the most "Pythonic" way would probably involve at least one if not of these not being classes you created, so it is a bit odd to review. Python doesn't lean as heavily on OOP as Java does but I hope the answers are helpful to you regardless. \$\endgroup\$ – SuperBiasedMan Sep 23 '15 at 15:25
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ thank you very much for your professional advice, and i will improve the code. \$\endgroup\$ – burce pan Sep 23 '15 at 15:47
  • \$\begingroup\$ @burcepan Glad to help. Please do ask if any part is confusing as I'm not sure how much I should explain when you're a beginner and a lot of this answer is conceptual. \$\endgroup\$ – SuperBiasedMan Sep 23 '15 at 15:51
  • \$\begingroup\$ @ SuperBiasedMan :I have learned your advise,thanks again. and your question"Why make cartlist a dictionary?",because I want use the following feature of dictionary. dict[key] can get the value of the key. for example,there is of dictionary {'apple':5, 'pear',6}, if i want to know the amount of the apple, i can use dict[apple] to achieve it. but if i use list , i must iterate the list to find apple \$\endgroup\$ – burce pan Sep 27 '15 at 6:57
  • \$\begingroup\$ @burcepan I see the confusion now. I edited my answer to explain how to use an attribute of Cart instead of making it inherit from dict. \$\endgroup\$ – SuperBiasedMan Sep 27 '15 at 12:25
  • \$\begingroup\$ @SuperBiasedMan Don't sub-class to extend functionality, instead make the parent a variable? That doesn't sound like OOP. Why should you inherit dict to then hold a different dict, why not just use self? When you want to extend functionality you will have to always use self.items rather than self. And finally Cart['a'] = 'a' is valid with your cart class, but will never be used by the class. \$\endgroup\$ – Peilonrayz Sep 27 '15 at 15:13
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Ethan Bierlein:thank you very much, I have learned PEP8 after you tell me this document. I do find some PEP8 violations. tanks again. \$\endgroup\$ – burce pan Sep 27 '15 at 6:00
1
\$\begingroup\$

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.

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.

\$\endgroup\$
  • \$\begingroup\$ thank you very much, and i need some time to Io understand your advice。 \$\endgroup\$ – burce pan Sep 24 '15 at 15:41
  • 1
    \$\begingroup\$ To elaborate: if you ever develop the need to intercept the access to the instance variables, you can later change them to properties, and write getters and setters. \$\endgroup\$ – 200_success Sep 27 '15 at 15:22

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.