Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I think I'm on the right track, but ANY suggestions or critques are welcome. The program just scrapes an HTML table and prints it to stdout.

import sys
from urllib2 import urlopen, URLError
from argparse import ArgumentParser
from bs4 import BeautifulSoup


def parse_arguments():
    """ Process command line arguments """
    parser = ArgumentParser(description='Grabs tables from html')
    parser.add_argument('-u', '--url', help='url to grab from',
                        required=True)
    args = parser.parse_args()
    return args


def parse_rows(rows):
    """ Get data from rows """
    results = []
    for row in rows:
        table_headers = row.find_all('th')
        if table_headers:
            results.append([headers.get_text() for headers in table_headers])

        table_data = row.find_all('td')
        if table_data:
            results.append([data.get_text() for data in table_data])
    return results


def main():
    # Get arguments
    args = parse_arguments()
    if args.url:
        url = args.url

    # Make soup
    try:
        resp = urlopen(url)
    except URLError as e:
        print 'An error occured fetching %s \n %s' % (url, e.reason)   
        return 1
    soup = BeautifulSoup(resp.read())

    # Get table
    try:
        table = soup.find('table')
    except AttributeError as e:
        print 'No tables found, exiting'
        return 1

    # Get rows
    try:
        rows = table.find_all('tr')
    except AttributeError as e:
        print 'No table rows found, exiting'
        return 1

    # Get data
    table_data = parse_rows(rows)

    # Print data
    for i in table_data:
       print '\t'.join(i)


if __name__ == '__main__':
    status = main()
    sys.exit(status)
share|improve this question

2 Answers 2

up vote 7 down vote accepted

In general, your code is very well written. PEP8 compliance is very good, names make sense, you have docstrings, you use if __name__ == '__main__:.

Here's a few comments on what I would do differently:

# Get table
try:
    table = soup.find('table')
except AttributeError as e:
    print 'No tables found, exiting'
    return 1

# Get rows
try:
    rows = table.find_all('tr')
except AttributeError as e:
    print 'No table rows found, exiting'
    return 1

This is essentially the same code duplicated twice. The fact that you differentiate between not finding a table and not finding rows within it is nice, but ultimately useless, as a table with nothing in it might as well be no table at all.

Therefore, I propose changing it to detect a valid table, that is one with rows. This tidies up the code a bit like so:

try:
    table = soup.find('table')
    rows = table.find_all('tr')
except AttributeError as e:
    raise ValueError("No valid table found")

You'll notice I eliminated the print and the return 1. Errors should be errors, they shouldn't magically become print statements. The only exception to this is if this is supposed to be for end users, then you will want to retain the print. Secondly, sys.exit() is unnecessary as the script will end automatically when it reaches the end of the file. Therefore, all the return 1's are unnecessary and are extra fluff.

Lastly, let's talk about your comments. Most of them are useless.

# Make soup

Is that really the best description? You are retrieving a url and reading the HTML. THAT should be your comment.

# Get table

# Get rows

As I said, I think these two can be wonderfully merged. Also, I'm pretty sure the names are more than sufficient to understand that you are retrieving the table and the rows of the table.

# Get data
table_data = parse_rows(rows)

Hmm... what else could table_data be? This is a comment that seems pretty useless and adds un-needed fluff.

# Print data

This one I actually think should be more descriptive, such as:

# Print table data 

As indicated in the comments, it is suggested that you remove all of your comments, as they are largely plain fluff considering your naming is so well done.

Another last thing to note is that

if args.url:

Is a completely useless check. It is guaranteed that args.url will always exist as per how the argparse module functions. If you made the url an optional parameter, that would be a different story, but then your program would be useless.

share|improve this answer
    
To be precise, the return 1 will become useless if he changes the script to raise errors instead of printing messages. I'd say all his non-docstring comments are pointless. And the if args.url is pointless too, isn't it? –  janos Aug 22 at 5:53
    
I agree. About the comments: I agree, but extra comments rarely hurt. I also agree that if args.url is indeed useless in the code's current state. –  mleyfman Aug 22 at 6:00
    
Extra comments hurt my eyes when they have no meaning. (same with people commenting setters and getters...) –  RvdK Aug 22 at 6:52
    
I've add a note that suggests removing all the trivial comments and points out the if args.url bit –  mleyfman Aug 22 at 6:58
    
Ah, I'm trying to transition from the mindset of writing end user scripts to modules and the likes so the errors vs. printing + returns is excellent advice. I will lay off the useless comments as well. Thanks much –  Creek Aug 22 at 17:49

Decomposition

I would prefer to see a parse_table(…) function rather than a parse_rows(…) function. That is, after all, a function that is closer to the essence of what this program accomplishes. Extracting the <tr> rows within the table is just a detail within that goal.

Error handling

I don't believe that BeautifulSoup raises AttributeError the way you expect. When .find() finds no matching elements, it just returns None. When .find_all() finds no matching elements, it returns an empty list.

Error messages should be printed to sys.stderr to avoid contaminating the sys.stdout, which should be reserved for outputting scraped data.

Output

I recommend calling .strip() on the strings returned by .get_text(), so that leading/trailing whitespace in the HTML source of each table cell doesn't mess up the output format. Otherwise, the printout from a table like this would be a mess:

<table>
  <tr>
    <th>
      Word
    </th>
    <th>
      Definition
    </th>
  </tr>
  <!-- etc -->
</table>

Consider adding some kind of quoting mechanism, so that the output can be in a useful tab-separated-values format even if the HTML source code happens to contain Tab or Newline characters.

Cell extraction

In your parse_rows(), you find all <th> elements, then all <td> elements. This is problematic for two reasons:

  1. Code duplication: both loops have essentially the same bodies.
  2. Element reordering: any <th> elements would be pulled to the beginning of the resulting list, before all <td> elements. While headers usually come before the data cells, you shouldn't make that assumption.

In my opinion, <tr> that contains no <th> or <td> descendants should still produce an empty element in the result. (You skip such empty rows.)

Taking into account all of the other advice above as well, I would personally write the cell extraction function this way, using list comprehensions.

def parse_table(table):
    """ Get data from table """
    return [
        [cell.get_text().strip() for cell in row.find_all(['th', 'td'])]
           for row in table.find_all('tr')
    ]
share|improve this answer
    
All excellent points, especially about the cell extraction and error handling. I didn't realize it was possible to search for multiple tags simultaneously. And that function is slick, I like that, alot. –  Creek Aug 22 at 15:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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