5
\$\begingroup\$

The script scrapes a table from the website mentioned, looks at the last 2 columns, takes that information, and then sorts it (and then returns the largest county, and the set of races and their numbers). But, basically, I just wanted to know in which ways I can make this script parallel more closely with the Zen of Python, and if there are any optimizations I could make to the script to make it a little faster. I also know that Python's goal is not to be fast, but if I can make the script faster, I don't see why not (though the main priority is to make it Pythonic).

#!/usr/bin/env python
"""Scraping information from https://www.tdcj.state.tx.us/death_row/dr_executed_offenders.html"""

import requests, time
from bs4 import BeautifulSoup
from collections import defaultdict

url = "https://www.tdcj.texas.gov/death_row/dr_executed_offenders.html"
county = {}


def counties(countyArg):
    """Fills the `county` variable"""
    f = open("counties.txt")
    for line in f.readlines():
        countyArg[line.replace('\n', '').rstrip()] = 0

def scrape(urlArg, countyArg):
    """Scrape Site Based on HTML"""
    races = defaultdict(int)

    page = requests.get(urlArg)
    html = page.content

    soup = BeautifulSoup(html, 'html.parser')
    table = soup.find('table')
    for row in table.findAll('tr')[1:]:
        if row.findAll('td')[8].text == 'Black':
            races['black'] += 1
        elif row.findAll('td')[8].text == 'White':
            races['white'] += 1
        elif row.findAll('td')[8].text == 'Hispanic':
            races['hispanic'] += 1

        countyArg[row.findAll('td')[9].text.encode('ascii').rstrip()] += 1

    return races, county

def find_largest(countyArg):
    """find largest county"""
    largestCounty = ''
    largestInmates = 0
    for key, value in countyArg.iteritems():
        if largestInmates < value:
            largestInmates = value
            largestCounty = key
    return largestCounty, largestInmates

if __name__ == '__main__':
    counties(county)
    print county
    scrape(url, county)
    print find_largest(county)
\$\endgroup\$
3
\$\begingroup\$

In addition to PEP 20, the Python Zen, which you seem to be already familiar with, another must read (and follow) for Python coders is PEP 8, the style guide. Many of those recommendations apply to your code, please read it carefully, I won't repeat them here.

Working with resources

In counties, you open a file for reading but you never close it. You should always close the resources, file handles you open. But actually, in Python, you should use the with syntax, like this:

with open("counties.txt") as fh:
    for line in fh.readlines():
        countyArg[line.replace('\n', '').rstrip()] = 0

In this form, you don't actually need to close: Python will automatically close the file handle after leaving the with block.

Why out-parameters?

It's not clear why you opted for initializing an empty county dictionary and then filling it in the counties method. It would be more natural to let counties create the dictionary and return it.

Avoid global variables

county is declared at the outermost scope, effectively making it a global variable. Avoid global variables as much as possible.

As hinted earlier, it would be better to create the dictionary inside a function, and pass it around to the functions that need it.

For example:

load_counties(path):
    counties = {}
    with open(path) as fh:
        for line in fh.readlines():
            counties[line.replace('\n', '').rstrip()] = 0

    return counties

# ....

def main():
    counties = load_counties("counties.txt")
    scrape(url, counties)
    # ....        

Avoid code duplication

Avoid repeated logic like this:

for row in table.findAll('tr')[1:]:
    if row.findAll('td')[8].text == 'Black':
        races['black'] += 1
    elif row.findAll('td')[8].text == 'White':
        races['white'] += 1
    elif row.findAll('td')[8].text == 'Hispanic':
        races['hispanic'] += 1

Not only it is error-prone to repeat row.findAll('td')[8].text many times, it's also inefficient: beautiful soup has to find all td elements multiple times.

Refactor to do this only once:

for row in table.findAll('tr')[1:]:
    race = row.findAll('td')[8].text
    if race == 'Black':
        races['black'] += 1
    elif race == 'White':
        races['white'] += 1
    elif race == 'Hispanic':
        races['hispanic'] += 1

We can also go one step further:

for row in table.findAll('tr')[1:]:
    race = row.findAll('td')[8].text
    if race in ('Black', 'White', 'Hispanic'):
        races[race.lower()] += 1

Consider using collections.Counter

The collections package has a class that can help you here: Counter. Instead of doing the counting by yourself, this handy little utility can do it for you, it goes something like this:

from collections import Counter

# ...

    races = []
    for row in table.findAll('tr')[1:]:
        race = row.findAll('td')[8].text
        if race in ('Black', 'White', 'Hispanic'):
            races.append(race.lower())

    return Counter(races)

That is, from a list like ['black', 'white', 'black', 'white', 'black', 'hispanic'] it will create Counter({'black': 3, 'white': 2, 'hispanic': 1})

Use max to find the maximum entry of a dictionary

find_largest can be simplified using max:

def find_largest(counties):
    return max(counties.items(), key=lambda pair: pair[1]))
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks, already changed the file opening method to the aforementioned suggested method. As for the county dictionary, I am using the variable in other functions, so if I let counties create it, would the other function be able to access it (without using any globals)? Also, how would I let counties() create the dict? \$\endgroup\$ – Scoutdrago3 Aug 7 '15 at 20:36
  • 1
    \$\begingroup\$ I added a note about that in the "Avoid global variables" section. I added a many other sections too. \$\endgroup\$ – janos Aug 7 '15 at 20:51
  • \$\begingroup\$ OK thanks. I have one question on the "Use max to find the maximum entry of a dictionary". What does the lambda do (I do know what a lambda is, but what is it doing there)? As for all the other suggestions, I have changed them out. I ran my code through a PEP8 checker a few times and fixed any errors that arrived. I'll go ahead and update all my code. EDIT: The max function only returns a string of the county. I was hoping to get both variables. Any way of doing that? EDIT: Not only does it only return the key, but it also returns the wrong county... \$\endgroup\$ – Scoutdrago3 Aug 7 '15 at 21:00
  • 1
    \$\begingroup\$ The lambda is called with each value in the iterable. It should return a value that will be used to determine the ordering. In this example, the iterable is a list of name-count tuples. So pair[1] corresponds to count, and so effectively the dictionary items will be ordered by value. When you say "it doesn't work", I think you're not using exactly what I gave you \$\endgroup\$ – janos Aug 7 '15 at 21:09
  • \$\begingroup\$ Ah apologies, missed a .items. Thanks for all the help. Shortened the code by 6 lines, and shortened the run time from ~2:60 to ~2:59 (haha), among other things. \$\endgroup\$ – Scoutdrago3 Aug 7 '15 at 21:14

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.