1
\$\begingroup\$

I'm looking for any and all feedback on quality, style and efficacy of the code. If there's a simple way to get the header put in, I'd love to hear that.

The code is used to take zip codes from a csv file and and spit back out the city and state where available.

import sys
import os
import csv
from bs4 import BeautifulSoup
import requests


def main():
    pass


def getCityState(zipCode):
# given zip code, return city and state if available
    zipCode = str(zipCode)
    url = "http://www.city-data.com/zips/" + zipCode + ".html"
    r = requests.get(url)
    data = r.text
    soup = BeautifulSoup(data)
    if soup.findAll(text="City:") ==[]:
        cityNeeded = soup.findAll(text="Cities:")
        for t in cityNeeded:
            return t.find_next('a').string

    else:
        cityNeeded = soup.findAll(text="City:")
        for t in cityNeeded:
            return t.find_next('a').string      


def getTableFromCSV(fileName, settings):
    with open(fileName, settings) as fp:
        reader = csv.DictReader(fp, skipinitialspace=True)
        table = [row for row in reader]
        header = reader.fieldnames
    return header,table # header and table are lists of dicts


def printTable(header, table):
# print table in console
    print"begin printTable"
    for row in table:
        print row
    print"end printTable"   

def addToList(theList, position, newItem):
    theList.insert(position, newItem)
    return theList

def addToDict(theDict, newKey, newItem):
    theDict[newKey] = newItem
    return theDict


def checkZips(table):
# check zips and insert the city and town if there is a good zip
    for row in table:
        stringNeed = row['zip']
        if not stringNeed.isdigit(): # check to see if it's a number
            row["cityState"] = "zip not number"
            pass
        else:
            if not len(stringNeed) == 5:  #check to see if it's 5 digits long
                row["cityState"] = "zip not 5 digits"
            else:
                cityState = getCityState(stringNeed)
                row["cityState"] = cityState
                # print row
     return table

if __name__ == '__main__':
    print"begin main"

    # get table from csv file
    header, table = getTableFromCSV("propertyOutputTest.csv","rUbw")

    # add city, state to header
    header.insert(0,"cityState")

    #check zips and insert city and state whree available
    checkZips(table)

    # write table to a csv file that replaces the old csv file
    with open("propertyOutputTest.csv","wb") as fp:
        writer = csv.DictWriter(fp, header)
        for row in table:
            writer.writerow(row)

    print"end main"
\$\endgroup\$
  • \$\begingroup\$ Just curious from someone in the beginning stages of python, but why pass in your main method but instead put everything inline? \$\endgroup\$ – Phix Dec 4 '13 at 1:51
  • \$\begingroup\$ i'm a n00b too. That was just in the textmate template. No idea why. \$\endgroup\$ – dwstein Dec 4 '13 at 2:14
  • \$\begingroup\$ @Phix explained that in my answer. \$\endgroup\$ – Quentin Pradet Dec 4 '13 at 8:35
4
\$\begingroup\$

First, a few general comments about the 'main function' in Python. The __name__ == '__main__' idiom is a way to distinguish between simply importing a module and launching one. You may want to simply import the module you just wrote to reuse the checkZips function elsewhere, but you don't want the main function to run when doing that.

Some people simply write:

if __name__ == '__main__':
    main()

to run a function called main() when running this module (eg. with python mymodule.py). This is only a convention, the function could be called whatever you want it to be, or you could simply put all the code in the conditional, as you've done here. TextMate provided you with a main() function with a pass statement to specify that it does nothing and avoid a syntax error. Long story short: remove that main function!

import sys
import os
import csv
from bs4 import BeautifulSoup
import requests

Your module lacks a docstring. It could contain the explanation you gave us. Read more about Python code conventions in PEP 8 and docstring conventions PEP 257.

def main():
    pass

Remove that, as said earlier.

def getCityState(zipCode):
# given zip code, return city and state if available

This should be a docstring.

    zipCode = str(zipCode)

Your zip code is already a string, you don't need to convert it first.

    url = "http://www.city-data.com/zips/" + zipCode + ".html"
    r = requests.get(url)
    data = r.text
    soup = BeautifulSoup(data)

You may not need all those variables, but that's up to you (eg. you could simply write BeautifulSoup(r.text)).

    if soup.findAll(text="City:") ==[]:

Be careful about indentation and follow PEP8 closely (eg. using a PEP 8 checker). Here you need a space after the ==, it makes the whole code easier to read and allows everyone to focus on the important matters.

        cityNeeded = soup.findAll(text="Cities:")
        for t in cityNeeded:
            return t.find_next('a').string

    else:
        cityNeeded = soup.findAll(text="City:")
        for t in cityNeeded:
            return t.find_next('a').string

First, you might want to know about list comprehensions, this can be written as return [t.find_next('a').string for t in cityNeeded]. Second, Don't repeat yourself! Having duplicated code increases the likelihood of errors, eg. when you only fix one part. It is easy here, you can simply move the loop out of the conditional. You can also use a regular expression to remove the conditional entirely (untested):

cityNeeded = soup.findAll(text=re.compile("City:|Cities:"))

Make sure to import re.

def getTableFromCSV(fileName, settings):
    with open(fileName, settings) as fp:
        reader = csv.DictReader(fp, skipinitialspace=True)
        table = [row for row in reader]
        header = reader.fieldnames
    return header,table # header and table are lists of dicts

Nothing much to say here, so I will talk about coding style. :) PEP 8 says that you should write get_table_from_csv. If you don't like it, you don't have to, it's just how most Python coders write code. Second, header,table should be header, table. Third, PEP 8 says you need a double space before inline comments (return header, table # ...). Fourth, this isn't really a comment, and would be a better fit in the docstring. Fifth, about indentation: don't mix tabs and spaces, and no trailing spaces after your lines. (You didn't have any trailing spaces in this function though).

def printTable(header, table):
# print table in console
    print"begin printTable"
    for row in table:
        print row
    print"end printTable"

print is statement in Python 2, but a function in Python 3. I'd advise you to switch to Python 3 since Python 2 will soon be the past of Python, and at least write things such as print("end printTable") to prepare yourself to write Python 3. There are a few major differences and many small differences between the two version, so the earlier you switch, the easier it will be to get acustomed to Python 3.

Also, remove debug prints when you don't need them anymore.

def addToList(theList, position, newItem):
    theList.insert(position, newItem)
    return theList

def addToDict(theDict, newKey, newItem):
    theDict[newKey] = newItem
    return theDict

Come on, you don't need a function for those!

Overall, don't be overwhelmed by my comments. The code is nice, but there a lots of little things to improve. :)

\$\endgroup\$
  • \$\begingroup\$ To use print() in Python 2 is dubious advice: consider for example the output of print "a", "b" vs. print("a", "b"). \$\endgroup\$ – Janne Karila Dec 4 '13 at 13:27
  • \$\begingroup\$ I'd answer from __future__ import print_function. \$\endgroup\$ – Quentin Pradet Dec 4 '13 at 13: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.