5
\$\begingroup\$

When I wrote this it just felt messy due to the null checks. Any good ideas on how to clean it up would be most appreciated.

 def getItemsInStock(self):
        itemsInStock = []
        items = self.soup.find_all("ul", {'class':re.compile('results.*')})
        getItems = [x for x in items if x.find("div", class_="quantity")]
        if getItems:
            itemsInStock += getItems
        pages = self.combThroughPages()
        if pages:
            for each in pages:
                self.uri = each
                soup = self.createSoup()
                items = soup.find_all("ul", {'class':re.compile('results.*')})
                if items:
                    inStock = [x for x in items if x.find("div", class_="quantity")]
                    if inStock:
                        itemsInStock += inStock
        if itemsInStock:
            return self.__returnItemDetailAsDictionary(itemsInStock)
        else:
            return None
\$\endgroup\$
1
  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. The current title states your concerns about the code; it needs an edit to simply state the task; see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Jun 22 '18 at 7:39
1
\$\begingroup\$

If a function like self.combThroughPages() returns None or a list of items you can wrap such a call in a function listify() that makes None into an empty list:

def listify(l):
    if l is None:
        return []

and make the call like:

...
for each in listify(self.combThroughPages())
    self.uri = each
...

This works even if you don't have control over the definition of combThroughPages().

Sometimes you have functions that return None, or a single item or a list. I use a slightly more elaborate version of listify() myself to handle that:

def listify(val, none=None):
    """return a list if val is only an element.
    if val is None, normally it is returned as is, however if none is set 
    and True then [None] is returned as a list, if none is set but not 
    True ( listify(val, False) ), then the empty list is returned.

    listify(None) == None
    listify(None, 1) == [1]
    listify(None, [2]) == [[2]]
    listify(None, False) == []
    listify(3) == [3]
    listify([4]) == [4]
    listify(5, 6) == [5]
    """
    if val is None:
        if none is None:
            return None
        elif none:
            return [None]
        else:
            return []
    if isinstance(val, list):
        return val
    return [val]

In your case that version would be called like:

...
for each in listify(self.combThroughPages(), False)
    self.uri = each
...

Since you are not further using the temporary lists getItems or inStock you can get rid of them and directly append items found to itemsInStock. This would get you (assuming you have the extended version of listify in your scope)

def getItemsInStock(self):
    itemsInStock = []
    for item in self.soup.find_all("ul", {'class':re.compile('results.*')}):
        if item.find("div", class_="quantity"):
            itemsInStock.append(item)
    for self.uri in listify(self.combThroughPages(), False):
        soup = self.createSoup()
        for item in listify(soup.find_all("ul", {'class':re.compile('results.*')}), False):
            if item.find("div", class_="quantity"):
                itemsInStock.append(item)
    if itemsInStock:
        return self.__returnItemDetailAsDictionary(itemsInStock)
    else:
        return None

It is of course impossible to test without context, but this should work.

I also removed the variable each directly setting self.uri. I can only assume that self.createSoup is dependent on the value of self.uri, otherwise I am not sure why you would have differences in calling createSoup.


Of course you don't need listify() around self.combThroughPages() if you change the latter to return an empty list as @ruds already proposed that would work as well. In that case I would probably also have getItemsInStock() return an empty dictionary ( return {} ) depending on how that function itself is called:

 iis = self.getItemsInStock()
 if iis:
     for key, value in iis.iteritems()

could then be changed to:

 for key, value in iis.iteritems():

(or you can write a dictify() function).

\$\endgroup\$
4
\$\begingroup\$

As an example,

    getItems = [x for x in items if x.find("div", class_="quantity")]
    if getItems:
        itemsInStock += getItems

can be written

    itemsInStock.extend(x for x in items if x.find("div", class_="quantity")]

extend (and for that matter, +=) are no-ops on empty lists.

Now, I'm not sure what some of the function you call return, but I don't like that getItemsInStock returns None if no items are in stock -- it makes more sense to return an empty list in that case. If your other functions did the same, you wouldn't need any of your if guards. Another example:

    pages = self.combThroughPages()
    if pages:
        for each in pages:
            ...

can be written

    for page in self.combThroughPages():
        ...

but only if combThroughPages returns an empty list when there are no, um, pages, I guess? (It's not clear from your function name.)

\$\endgroup\$
1
  • 2
    \$\begingroup\$ And finally, change __returnItemDetailAsDictionary to accept an empty list for which it returns an empty dictionary. Now you can drop the if itemsInStock block and simply return self.__returnItemDetailAsDictionary(itemsInStock) in all cases. \$\endgroup\$ – David Harkness Jun 17 '13 at 0:48
4
\$\begingroup\$

One of my favourite Python shorthands is the or operator being used to provide a default value:

x = y or 42

This will assign the value of y to x, if y evaluates to True when interpreted as a boolean, or 42 otherwise. While this may seem odd at first, it is perfectly normal if you consider short-circuit evaluation and how Python can interpret nearly everything as a boolean: If the first operand evaluates to True (e.g., it is not None), return the first operand, otherwise return the second.

In other words, A or B is equivalent to A if A else B, and conversely (but not quite as intuitive, I guess), A and B is equivalent to B if A else A.

This is not only handy for assigning default values to variables, as in the above example, but can also be used to get rid of some of your if checks. For example, instead of

pages = self.combThroughPages()
if pages:
    for each in pages:
        # do stuff

you can write:

pages = self.combThroughPages() or []
for each in pages:
    # do stuff

or even shorter, since now you need pages only once:

for each in self.combThroughPages() or []:
    # do stuff

Now, if combThroughPages() returns None (which is False when interpreted as a boolean), the or operator will return the second operand: [].

(Of course, if combThroughPages() returns an empty list instead of None, you don't have to check at all, as pointed out by @ruds; same for your list comprehensions.)

\$\endgroup\$
3
  • \$\begingroup\$ I'm a big fan of None or [] as a way of unifying return values - in the Maya api, which is extremely inconsistent about returning None, single items, or lists it's a lifesaver \$\endgroup\$ – theodox Jul 2 '13 at 4:31
  • \$\begingroup\$ y = 0 will evaluate in x = y or 42 to x = 42. Is this ok? \$\endgroup\$ – Elmex80s Jan 6 '18 at 11:35
  • \$\begingroup\$ @Elmex80s What do you mean with "is this okay"? If a function returns None or numeric values, including 0, then yes, that's a very good point where using or can lead to unexpected behavior. But in OP's case, that should not be a problem. \$\endgroup\$ – tobias_k Jan 6 '18 at 16:37

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