7
\$\begingroup\$

This was largely an exercise in making my code more Pythonic, especially in catching errors and doing things the right way.

I opted to make the PageNotFound exception part of the class so that users could simply from ratebeer import RateBeer and not have to worry about anything else.

If you prefer, the code is on Github.

from bs4 import BeautifulSoup
import requests
import re

import exceptions

class RateBeer():
    """
    Makes getting information about beers and breweries from RateBeer.com as easy as:
    >>> summit_epa = RateBeer().beer("summit extra pale ale")
    A utility for searching RateBeer.com, finding information about beers, breweries, and reviews.
    The nature of web scraping means that this package is offered in perpetual beta.
    Requires BeautifulSoup, Requests, and lxml.
    See https://github.com/alilja/ratebeer for the full README.
    """

    class PageNotFound(Exception):
        pass

    def __init__(self):
        self.BASE_URL = "http://www.ratebeer.com"

    def _search(self, query):
        # this feels bad to me
        # but if it fits, i sits
        payload = {"BeerName": query}
        r = requests.post(self.BASE_URL+"/findbeer.asp", data = payload)
        return BeautifulSoup(r.text, "lxml")

    def _parse(self, soup):
        s_results = soup.find_all('table',{'class':'results'})
        output = {"breweries":[],"beers":[]}
        beer_location = 0
        # find brewery information
        if any("brewers" in s for s in soup.find_all("h1")):
            s_breweries = s_results[0].find_all('tr')
            beer_location = 1
            for row in s_breweries:
                location = row.find('td',{'align':'right'})
                output['breweries'].append({
                    "name":row.a.contents,
                    "url":row.a.get('href'),
                    "id":re.search("/(?P<id>\d*)/",row.a.get('href')).group('id'),
                    "location":location.text.strip(),
                })
        # find beer information
        if any("beers" in s for s in soup.find_all("h1")) or not soup.find_all(text="0 beers"):
            s_beer_trs = iter(s_results[beer_location].find_all('tr'))
            next(s_beer_trs)
            for row in s_beer_trs: 
                link = row.find('td','results').a
                align_right = row.find_all("td",{'align':'right'})
                output['beers'].append({
                        "name":link.text,
                        "url":link.get('href'),
                        "id":re.search("/(?P<id>\d*)/",link.get('href')).group('id'),
                        "rating":align_right[-2].text.strip(),
                        "num_ratings":align_right[-1].text,
                    })
        return output

    def search(self, query):
        return self._parse(self._search(query))

    def beer(self, url):
        r = requests.get(self.BASE_URL+url, allow_redirects=True)
        soup = BeautifulSoup(r.text,"lxml")
        output = {}

        # check for 404s
        try:
            s_contents_rows = soup.find('div',id='container').find('table').find_all('tr')
        except AttributeError:
            raise RateBeer.PageNotFound(url)
        # ratebeer pages don't actually 404, they just send you to this weird "beer reference" 
        # page but the url doesn't actually change, it just seems like it's all getting done
        # server side -- so we have to look for the contents h1 to see if we're looking at the
        # beer reference or not
        if "beer reference" in s_contents_rows[0].find_all('td')[1].h1.contents:
            raise RateBeer.PageNotFound(url)

        info = s_contents_rows[1].tr.find_all('td')
        additional_info = s_contents_rows[1].find_all('td')[1].div.small
        abbr = additional_info.find_all(["abbr","a"])
        big = additional_info.find_all("big")
        if additional_info.find(text=re.compile("SEASONAL")):
            del big[2]
        for location, label in enumerate(abbr):
            key = None
            if "RATINGS" in label.text:  key = "num_ratings"
            if "CALORIES" in label.text: key = "calories"
            if "ABV" in label.text:      key = "abv"
            if "SEASONAL" in label.text: key = "season"
            if "IBU" in label.text:      key = "ibu"

            if key is not None:
                output[key] = big[location].text

        output.update({'name':s_contents_rows[0].find_all('td')[1].h1.text,
            'overall_rating':info[0].find_all('span', attrs={'itemprop':'average'})[0].text,
            'style_rating':info[0].find_all('div')[2].div.span.text,
            'brewery': info[1].a.text,
            'brewery_url':info[1].a.get('href'),
            'style':info[1].div.find_all('a')[1].text,
        })
        return output

    def reviews(self, url, pages=1,start_page=1,review_order="most recent"):
        assert pages > 0, "``pages`` must be greater than 0"
        assert start_page > 0, "``start_page`` must be greater than 0"
        review_order = review_order.lower()
        url_codes = {
            "most recent":1,
            "top raters":2,
            "highest score":3
            }
        url_flag = url_codes.get(review_order)
        if not url_flag: raise ValueError, "Invalid ``review_order``."

        output = []
        for page_number in range(start_page,start_page+pages):
            complete_url = "{0}{1}{2}/{3}/".format(self.BASE_URL, url, url_flag, page_number)
            r = requests.get(complete_url, allow_redirects=True)
            soup = BeautifulSoup(r.text, "lxml")
            content = soup.find('div',id='container').find('table').find_all('tr')[5]
            [x.extract() for x in content.find_all('table')] # strip ad section
            review_tuples = zip(*[iter(content.find_all('div'))] * 4) # basically magic
            for review in review_tuples:
                detail_tuples = zip(*[iter(review[0].find_all(["big","small"]))] * 2)
                details = dict([(
                        label.text.lower().strip().encode("ascii","ignore"), 
                        rating.text,
                    ) for (label, rating) in detail_tuples])
                details.update({'text':review[3].text})
                output.append(details)
        return output


    def brewery(self, url, include_beers=True):
        r = requests.get(self.BASE_URL+url, allow_redirects=True)
        soup = BeautifulSoup(r.text, "lxml")
        try:
            s_contents = soup.find('div',id='container').find('table').find_all('tr')[0].find_all('td')
        except AttributeError:
            raise RateBeer.PageNotFound(url)

        output = {
            'name':s_contents[8].h1.text,
            'type':re.search("Type: +(?P<type>[^ ]+)",s_contents[8].find_all('span','beerfoot')[1].text).group('type'),
            'street':s_contents[0].find('span',attrs={'itemprop':'streetAddress'}).text,
            'city':s_contents[0].find('span',attrs={'itemprop':'addressLocality'}).text,
            'state':s_contents[0].find('span',attrs={'itemprop':'addressRegion'}).text,
            'country':s_contents[0].find('span',attrs={'itemprop':'addressCountry'}).text,
            'postal_code':s_contents[0].find('span',attrs={'itemprop':'postalCode'}).text,
            }
        if include_beers:
            output.update({'beers':[]})
            s_beer_trs = iter(s_contents[8].find('table','maintable nohover').find_all('tr'))
            next(s_beer_trs)
            for row in s_beer_trs:
                beer = {
                    'name':row.a.text,
                    'url':row.a.get('href'),
                    'id':re.search("/(?P<id>\d*)/",row.a.get('href')).group('id'),
                    'rating':row.find_all('td')[4].text.strip(),
                    'num_ratings':row.find_all('td')[6].text.strip(),
                }
                output['beers'].append(beer)
        return output
\$\endgroup\$
  • 1
    \$\begingroup\$ It's an interesting first question. I got thirsty just thinking about it. :) \$\endgroup\$ – Edward Nov 15 '14 at 0:38
5
\$\begingroup\$

I ran pylint on your code, which came up with 98 warnings - this is not good! I have split this down into each warning class, ordered by number of occurrences.

Note that if you particularly disagree with any specific warning, you can use comments to disable them, e.g.:

# pylint: disable=too-many-locals

This at least makes it clear that you've considered and dismissed the issue.


bad-whitespace [30 counts]

The style guide lays out the rules for whitespace, you should have:

  • One space after commas (e.g. in tuples, lists, definitions and calls);
  • One space after colons (e.g. in dictionaries); and
  • No spaces around = in calls/definitions.

Not only do you not follow this, you are inconsistent. Also, lining up e.g.

if "RATINGS" in label.text:  key = "num_ratings"
if "CALORIES" in label.text: key = "calories"

with whitespace is not Pythonic (we will come back to this line later...)


line-too-long [24 counts]

Lines should be at most 79 characters long (72 for docstrings). One is 119 characters long!


bad-continuation [13 counts]

When continuing statements across multiple lines, you should either

acceptable = ('start on the first line' and
              'continue at the same indent') # close paren on same line

or

also_acceptable = (
    'start on the next line' and
    ' use a single level of indent'
) # closing paren on next line, here
#   ) or here

Pick one of the three, and be consistent.


missing-docstring [8 counts]

Although you have a docstring for the class (albeit not correctly formatted), none of its methods are documented.

I would put your overall explanation as a module docstring, then have an addition docstring for at least every public method. I like the Google style, but there are others.


multiple-statements [6 counts]

To return, as promised, to:

if "RATINGS" in label.text:  key = "num_ratings"

That line contains two statements, and should therefore be on two lines:

if "RATINGS" in label.text:  
    key = "num_ratings"

Note that you could use a dictionary to simplify this, for example:

KEYWORDS = {'RATINGS': 'num_ratings', ...}
for keyword in KEYWORDS:
    if keyword in label.text:
        key = KEYWORDS[keyword]
        break

invalid-name [5 counts]

A few methods use r as a variable name - something more meaningful might be helpful (pylint generally requires at least three characters).

Also, BASE_URL should be a class attribute.


trailing-whitespace [3 counts]

There should be no tabs/spaces on blank lines or at the end of a line. You can generally set up IDEs to remove this automatically (or when you ask them to - e.g. Format > Strip trailing whitespace in IDLE). If you have the option, turning on display of whitespace helps you see what's going on.


anomalous-backslash-in-string [3 counts]

You have a few regular expressions that use backslashes (e.g. for digits \d) - these should be made raw strings r'...'.


unused-import [1 count]

You never use exceptions


too-many-locals [1 count]

There are 20 local variables in RateBeer.beer, suggesting it should be split into more, smaller methods (pylint sets the limit at 15).


old-style-class [1 count]

RateBeer should inherit from object to make it a new-style class:

class RateBeer(object):

old-raise-syntax [1 count]

You mostly raise correctly, but:

if not url_flag: raise ValueError, "Invalid ``review_order``."

Again, one statement per line, and use the new syntax:

if not url_flag:
    raise ValueError("Invalid ``review_order``.")

no-self-use [1 count]

RateBeer._parse doesn't actually use any instance attributes, and could therefore be a standalone function or @staticmethod.


expression-not-assigned [1 count]

This line:

[x.extract() for x in content.find_all('table')] 

creates a list that never gets assigned. If you're just doing this for the side-effects, make that clear with e.g.:

_ = [x.extract() for x in content.find_all('table')] 

Additionally, the import statements should be:

import re # standard library first

from bs4 import BeautifulSoup # then third-party
import requests
\$\endgroup\$
  • 1
    \$\begingroup\$ Yeay, pylint, didn't know that one! Thanks! +1 \$\endgroup\$ – janos Nov 15 '14 at 12:55
  • \$\begingroup\$ The r variables have a very clear and simple role. You see where the value comes from, and it is used on the next line. I think a single-character name is totally appropriate. \$\endgroup\$ – Janne Karila Nov 16 '14 at 11:08
5
\$\begingroup\$

Coding style

It's a bit hard to read this code because it doesn't follow PEP8. The violations that stick in the eye the most:

  • No spacing around commas:
    • bad : {"breweries":[],"beers":[]}
    • good: {"breweries": [], "beers": []}
  • No line breaks after :, and unconventional spacing in if statements, for example in if "ABV" in label.text: key = "abv"

There are everywhere in the code. I suggest to get the pep8 command line tool (pip install pep8), run it on your project and correct all the violations.

Even with all PEP8 violations fixed, the could would benefit from more generous use of vertical spacing. For example the beer and reviews methods are too dense. It would be better to put some blank lines occasionally to create a sense of visual grouping of tightly related code, separating from loosely related ones.

Mutually exclusive if statements

It seems to me that these conditions are mutually exclusive:

if "RATINGS" in label.text:  key = "num_ratings"
if "CALORIES" in label.text: key = "calories"
if "ABV" in label.text:      key = "abv"
if "SEASONAL" in label.text: key = "season"
if "IBU" in label.text:      key = "ibu"

As such, it's a waste to make the program evaluate them all unnecessarily. These should be chained together with elif.

Don't repeat yourself

This piece of code appears in many places:

soup = BeautifulSoup(r.text, "lxml")

It would be better to create a helper method for this:

def get_soup(text):
    return BeautifulSoup(text, "lxml")

Other issues

Remove unused imports:

import exceptions

This docstring is wrong:

>>> summit_epa = RateBeer().beer("summit extra pale ale")

Should have been:

>>> RateBeer().search("summit extra pale ale")
>>> summit_epa = RateBeer().beer("/beer/summit-extra-pale-ale/7344/")

Modern style classes should extend object:

class RateBeer(object):
\$\endgroup\$
0
\$\begingroup\$

The phrase any(x in s for s in ...) can just be x in ....

The multiple copies of s_contents[0].find(...) can be minified in some ways, e.g. a separate function extracting that info and returning or modifying the output dict directly. In general splitting you functions up into smaller pieces may help to both understand and reuse parts of the functionality.

You do that in most places already, so I think adding more info to the other exceptions (url, review_order) would help debugging even more.

For packaging the setup.py doesn't list dependencies and pip install -e ratebeer failed for me:

  File ".../ratebeer/setup.py", line 8, in <module>

    import ratebeer

  File "./ratebeer/__init__.py", line 1, in <module>

    from ratebeer import RateBeer

ImportError: cannot import name RateBeer

So just include the full long description (or rather, cut it to one sentence or so), so that installation is possible in all cases.

Oh yeah and you could investigate Python2/3 compatibility as well.

\$\endgroup\$
  • \$\begingroup\$ any(x in ...) would fail on TypeError. And e.g. 'foo' in ('food', 'drink') has a very different outcome to ['foo' in s for s in ('food', 'drink')] \$\endgroup\$ – jonrsharpe Nov 15 '14 at 17:58
  • \$\begingroup\$ Thanks, edited. find_all returns a list though. \$\endgroup\$ – ferada Nov 15 '14 at 18:07
  • \$\begingroup\$ Yes, but my point is that the current code finds out whether 'beer' is in any of the strings in that list, whereas your suggested modification finds out whether any of the strings in that list is 'beer' - they aren't strictly equivalent. \$\endgroup\$ – jonrsharpe Nov 15 '14 at 18:08
  • \$\begingroup\$ Oh. Yeah I'll remove that part. \$\endgroup\$ – ferada Nov 15 '14 at 18:09
  • \$\begingroup\$ I wound up writing a little method inside brewery that just pulls out the span info. Cheers! I'm also not sure what you mean by including the full long description? Is that in setup.py? How do I include the dependencies in that? \$\endgroup\$ – AFL Nov 15 '14 at 22:48

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.