# Scrape an HTML table with python

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_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

# 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)


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.

# 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.

• 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? Aug 22, 2014 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. Aug 22, 2014 at 6:00
• Extra comments hurt my eyes when they have no meaning. (same with people commenting setters and getters...)
– RvdK
Aug 22, 2014 at 6:52
• I've add a note that suggests removing all the trivial comments and points out the if args.url bit Aug 22, 2014 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 Aug 22, 2014 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')
]

• 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. Aug 22, 2014 at 15:56