views:

107

answers:

3

I'm new to Python. Am I doing it right?

The goal:

  1. Get a bunch of InCart objects from Google App Engine's datastore that corresponds to the current user. Each InCart item has two attributes: InCart.owner and InCart.item

  2. Provide the templating engine a set of items in any InCart.item

The code:

    cartEntries = InCart.gql("WHERE owner = :1", self.user)
    items = []
    for entry in cartEntries:
        items.append(entry.item) 

    if items:
        render('Views/table.html', self, {'items': items})
    else:
        render('Views/message.html', self, {'msg': 'Your cart is empty.'})

This appears to work in my extremely brief testing. How am I doing in terms of style, code clarity, brevity, stability, etc?

+7  A: 

For:

items = []
for entry in cartEntries:
    items.append(entry.item) 

you could use a list comprehension:

items = [entry.item for entry in cartEntries]

Seasoned Python programmers find it easier to read and it's fewer lines.

Alok
+1. I read too quickly and thought they just wanted all the entries... not entry.item. I'm trying to delete my answer... Looks like others have to vote to delete it though.
Tom
A: 

Definitely use a comprehension as Alok said. I'd be inclined to use the same template for the table and add logic to the template to either render the table or the message, but that depends on the template system you are using.

Maybe you are using Views/message.html in a bunch of places so it makes sense to keep it separate.

cartEntries = InCart.gql("WHERE owner = :1", self.user)
items = [entry.item for entry in cartEntries]
render('Views/table.html', self, {'items': items, 'msg': 'Your cart is empty.'})
gnibbler
+1  A: 

Iterating over a query causes it to fetch results in batches of 20. This is much less efficient than fetching them all at once. You should call .fetch(), like this:

cartEntries = InCart.gql("WHERE owner = :1", self.user).fetch(1000)

Where 1000 is the maximum number of results you want.

If 'item' is a ReferenceProperty, you're also doing a datastore roundtrip for each entry in the cart. There's a way around that, but it only applies if it is a reference property.

Nick Johnson