6
\$\begingroup\$

How do I make my Python program faster?

I have 3 suspects right now for it being so slow:

  1. Maybe my computer is just slow

  2. Maybe my Internet is too slow (sometimes my program has to download the html of web pages and then it searches through the html for a specific piece of text)

  3. My code is slow (too many loops maybe? something else? I'm new to this so I wouldn't know!)

My code uses lots of loops I think...ALSO, another thing is that for the program to work you have to be logged in to this website: http://www.locationary.com/

from urllib import urlopen
from gzip import GzipFile
from cStringIO import StringIO
import re
import urllib
import urllib2
import webbrowser
import mechanize
import time
from difflib import SequenceMatcher
import os

def download(url):
    s = urlopen(url).read()
    if s[:2] == '\x1f\x8b': # assume it's gzipped data
        with GzipFile(mode='rb', fileobj=StringIO(s)) as ifh:
            s = ifh.read()
    return s

s = download('http://www.locationary.com/place/en/US/Utah/Provo-page5/?ACTION_TOKEN=NumericAction')

findLoc = re.compile('http://www\.locationary\.com/place/en/US/.{1,50}/.{1,50}/.{1,100}\.jsp')

findLocL = re.findall(findLoc,s)

W = []

X = []

XA = []

Y = []

YA = []

Z = []

ZA = []

for i in range(0,25):

    b = download(findLocL[i])

    findYP = re.compile('http://www\.yellowpages\.com/')

    findYPL = re.findall(findYP,b)

    findTitle = re.compile('<title>(.*) \(\d{1,10}.{1,100}\)</title>')

    getTitle = re.findall(findTitle,b)

    findAddress = re.compile('<title>.{1,100}\((.*), .{4,14}, United States\)</title>')

    getAddress = re.findall(findAddress,b)

    if not findYPL:

        if not getTitle:

            print ""

        else:

            W.append(findLocL[i])

        b = download(findLocL[i])

        if not getTitle:

            print ""

        else:

            X.append(getAddress)

        b = download(findLocL[i])

        if not getTitle:

            print ""

        else:

            Y.append(getTitle)

sizeWXY = len(W)

def XReplace(text, dic):
    for i, j in dic.iteritems():
        text = text.replace(i, j)  
    XA.append(text)

def YReplace(text2, dic2):
    for k, l in dic2.iteritems():
        text2 = text2.replace(k, l)  
    YA.append(text2)

for d in range(0,sizeWXY):

    old = str(X[d])

    reps = {' ':'-', ',':'', '\'':'', '[':'', ']':''}

    XReplace(old, reps)

    old2 = str(Y[d])

    YReplace(old2, reps)

count = 0

for e in range(0,sizeWXY):

    newYPL = "http://www.yellowpages.com/" + XA[e] + "/" + YA[e] + "?order=distance"

    v = download(newYPL)

    abc = str('<h3 class="business-name fn org">\n<a href="')

    dfe = str('" class="no-tracks url "')

    findFinal = re.compile(abc + '(.*)' + dfe)

    getFinal = re.findall(findFinal, v)

    if not getFinal:

        W.remove(W[(e-count)])

        X.remove(X[(e-count)])

        count = (count+1)

    else:

        for f in range(0,1):

            Z.append(getFinal[f])

XA = []

for c in range(0,(len(X))):

    aGd = re.compile('(.*), .{1,50}')

    bGd = re.findall(aGd, str(X[c]))

    XA.append(bGd)

LenZ = len(Z)

V = []

for i in range(0,(len(W))):

    if i == 0:

        countTwo = 0

    gda = download(Z[i-(countTwo)])

    ab = str('"street-address">\n')

    cd = str('\n</span>')

    ZAddress = re.compile(ab + '(.*)' + cd)

    ZAddress2 = re.findall(ZAddress, gda)

    for b in range(0,(len(ZAddress2))):

        if not ZAddress2[b]:

            print ""

        else:

            V.append(str(ZAddress2[b]))

            a = str(W[i-(countTwo)])

            n = str(Z[i-(countTwo)])

            c = str(XA[i])

            d = str(V[i])

            #webbrowser.open(a)

            #webbrowser.open(n)

            m = SequenceMatcher(None, c, d)

            if m.ratio() < 0.50:

                Z.remove(Z[i-(countTwo)])

                W.remove(W[i-(countTwo)])

                countTwo = (countTwo+1)

#LenZ2 = LenZ - (len(Z))

#if len(Z) == LenZ:

    #print "All of the Yellowpages were correct!"

#else:

    #if LenZ2 > 1:

        #print str(LenZ2) + " of the Yellowpages were incorrect!"

    #else:

        #print str(LenZ2) + " of the Yellowpages was incorrect!"

def ZReplace(text3, dic3):
    for p, q in dic3.iteritems():
        text3 = text3.replace(p, q)  
    ZA.append(text3)

for y in range(0,len(Z)):

    old3 = str(Z[y])

    reps2 = {':':'%3A', '/':'%2F', '?':'%3F', '=':'%3D'}

    ZReplace(old3, reps2)

#br = mechanize.Browser()

for z in range(0,len(ZA)):

    findPID = re.compile('\d{5,20}')

    getPID = re.findall(findPID,str(W[z]))

    newPID = re.sub("\D", "", str(getPID))

    finalURL = "http://www.locationary.com/access/proxy.jsp?ACTION_TOKEN=proxy_jsp$JspView$SaveAction&inPlaceID=" + str(newPID) + "&xxx_c_1_f_987=" + str(ZA[z])

    webbrowser.open(finalURL)

    time.sleep(5)

os.system("taskkill /F /IM firefox.exe")
\$\endgroup\$
  • 1
    \$\begingroup\$ What do you mean by "very slow"? Does it take hours, days? How long does it take and what were you expecting? \$\endgroup\$ – amit Dec 24 '11 at 16:56
14
\$\begingroup\$
from urllib import urlopen
from gzip import GzipFile
from cStringIO import StringIO
import re
import urllib
import urllib2
import webbrowser
import mechanize
import time
from difflib import SequenceMatcher
import os

def download(url):
    s = urlopen(url).read()
    if s[:2] == '\x1f\x8b': # assume it's gzipped data
        with GzipFile(mode='rb', fileobj=StringIO(s)) as ifh:
            s = ifh.read()
    return s

I'd probably pick a more descriptive name then s

s = download('http://www.locationary.com/place/en/US/Utah/Provo-page5/?ACTION_TOKEN=NumericAction')

findLoc = re.compile('http://www\.locationary\.com/place/en/US/.{1,50}/.{1,50}/.{1,100}\.jsp')

For constants, its usually best practice to name them in ALL_CAPS and put them outside of the actual logic. The logic in your program should really be in a main function not at the module level.

findLocL = re.findall(findLoc,s)

Not a very descriptive name, perhaps you can come up with a better one.

W = []

X = []

XA = []

Y = []

YA = []

Z = []

ZA = []

Don't double-space your code. Also, these variables are either really badly name or shouldn't be seperate variables. Possibly both.

for i in range(0,25):

Why 25? Do you want everything in findLocL? Then you should use for loc in findLocL.

    b = download(findLocL[i])

    findYP = re.compile('http://www\.yellowpages\.com/')

    findYPL = re.findall(findYP,b)

You don't have to call compile, you can just pass the regular expression as the first parameter. Compiling is only helpful if you use the compiled expression multiple times. Also, aren't you just searching for a simple string here? Why use regular expressions?

    findTitle = re.compile('<title>(.*) \(\d{1,10}.{1,100}\)</title>')

    getTitle = re.findall(findTitle,b)

    findAddress = re.compile('<title>.{1,100}\((.*), .{4,14}, United States\)</title>')

    getAddress = re.findall(findAddress,b)

    if not findYPL:

        if not getTitle:

Why did you findall if you just endup checking for a single match?

            print ""

        else:

            W.append(findLocL[i])

What the fried monkey is W? What you are searching for? You code is difficult to follow.

        b = download(findLocL[i])

        if not getTitle:

            print ""

        else:

            X.append(getAddress)

        b = download(findLocL[i])

So you just downloaded this file a few lines before. Why are you downloading it again? And why don't you ever do anything with the b you get?

        if not getTitle:

            print ""

        else:

            Y.append(getTitle)

sizeWXY = len(W)

Was that really helpful?

def XReplace(text, dic):
    for i, j in dic.iteritems():
        text = text.replace(i, j)  
    XA.append(text)

def YReplace(text2, dic2):
    for k, l in dic2.iteritems():
        text2 = text2.replace(k, l)  
    YA.append(text2)

These functions do pretty much the same thing. The only difference is that they both modify global variables. You shouldn't really be modifying global variables anyways.

for d in range(0,sizeWXY):

    old = str(X[d])

So... sizeWXY is len(W) but here you are fetching from X, what the fried turkey?

    reps = {' ':'-', ',':'', '\'':'', '[':'', ']':''}

    XReplace(old, reps)

    old2 = str(Y[d])

    YReplace(old2, reps)

count = 0

for e in range(0,sizeWXY):

    newYPL = "http://www.yellowpages.com/" + XA[e] + "/" + YA[e] + "?order=distance"

    v = download(newYPL)

    abc = str('<h3 class="business-name fn org">\n<a href="')

Why are constructing a string from a string?

    dfe = str('" class="no-tracks url "')

    findFinal = re.compile(abc + '(.*)' + dfe)

    getFinal = re.findall(findFinal, v)

    if not getFinal:

        W.remove(W[(e-count)])

Use del W[e-count]

        X.remove(X[(e-count)])

        count = (count+1)

No need for those parens

    else:

        for f in range(0,1):

            Z.append(getFinal[f])

So... that loop only executes once. Why is it a loop?

XA = []

Rather then reusing XA, I'd suggest working with a new list here.

for c in range(0,(len(X))):

    aGd = re.compile('(.*), .{1,50}')

    bGd = re.findall(aGd, str(X[c]))

bGd? seriously? What the fried turkey marshmallow duck is that supposed to be?

    XA.append(bGd)

LenZ = len(Z)

Don't do this. You are probably doing this because if you don't in C, it'll recalculate the length constantly as you execute a loop. Python doesn't do that.

V = []

for i in range(0,(len(W))):

Here you do pass the length to range. But you don't need those parens around it.

    if i == 0:

        countTwo = 0

Why don't you do this before the loop starts? That should be the same

    gda = download(Z[i-(countTwo)])

No need for the parens around countTwo.

    ab = str('"street-address">\n')

    cd = str('\n</span>')

    ZAddress = re.compile(ab + '(.*)' + cd)

    ZAddress2 = re.findall(ZAddress, gda)

    for b in range(0,(len(ZAddress2))):

In python you should almost never use a range loop unless you are actually counting. If you are looping with a length of something, you should almost always be looping over the list not range.

        if not ZAddress2[b]:

            print ""

        else:

            V.append(str(ZAddress2[b]))

            a = str(W[i-(countTwo)])

            n = str(Z[i-(countTwo)])

            c = str(XA[i])

            d = str(V[i])

So many hopelessly badly named variables. It hurts!

            #webbrowser.open(a)

            #webbrowser.open(n)

            m = SequenceMatcher(None, c, d)

Ok, I'm impressed that you know about and use this.

            if m.ratio() < 0.50:

                Z.remove(Z[i-(countTwo)])

                W.remove(W[i-(countTwo)])

                countTwo = (countTwo+1)

#LenZ2 = LenZ - (len(Z))



#if len(Z) == LenZ:

    #print "All of the Yellowpages were correct!"

#else:

    #if LenZ2 > 1:

        #print str(LenZ2) + " of the Yellowpages were incorrect!"

    #else:

        #print str(LenZ2) + " of the Yellowpages was incorrect!"

Don't keep dead code in comments

def ZReplace(text3, dic3):
    for p, q in dic3.iteritems():
        text3 = text3.replace(p, q)  
    ZA.append(text3)

Deja vu!

for y in range(0,len(Z)):

    old3 = str(Z[y])

    reps2 = {':':'%3A', '/':'%2F', '?':'%3F', '=':'%3D'}

    ZReplace(old3, reps2)

When you find yourself repeating the same code several times, its a sign you need to refactor

#br = mechanize.Browser()

for z in range(0,len(ZA)):

    findPID = re.compile('\d{5,20}')

    getPID = re.findall(findPID,str(W[z]))

    newPID = re.sub("\D", "", str(getPID))

    finalURL = "http://www.locationary.com/access/proxy.jsp?ACTION_TOKEN=proxy_jsp$JspView$SaveAction&inPlaceID=" + str(newPID) + "&xxx_c_1_f_987=" + str(ZA[z])

    webbrowser.open(finalURL)

    time.sleep(5)

os.system("taskkill /F /IM firefox.exe")

Stylistically, your code has a few issues which I've mentioned above. But in terms of performance, you are downloading web pages. The downloads will take way longer then the code so any speed issues in your code itself will not be noticeable.

I did point a couple of cases where you are downloading files you don't actually do anything with. Getting rid of those will help your code be faster.

You are trying to pull things out of HTML using regular expressions. This is a bad idea. It'll be brittle, hard to get right, and hard to read. Instead you should use something like BeautifulSoup: http://www.crummy.com/software/BeautifulSoup/. It'll parse HTML, (even broken HTML) and make it easy to pull out the pieces you are interested in.

The way to speed up the code is to fetch multiple web pages at the same time, and to process once web page while waiting for the others to download. The easiest way to do this is with the eventlet library.

Here is my rewrite of your code. I make no guarantees of correctness and I know it doesn't do the error handling it should. It also has too much stuff going on in the main function. But it doesn't suffer from the same speed problems. I'm just trying to give a hint at how your code could be easier to follow and not have the same performance problems.

from gzip import GzipFile
from cStringIO import StringIO
import re
import webbrowser
import time
from difflib import SequenceMatcher
import os
import sys
from BeautifulSoup import BeautifulSoup
import eventlet
from eventlet.green import urllib2

START_URL = 'http://www.locationary.com/place/en/US/Utah/Provo-page5/?ACTION_TOKEN=NumericAction'
TITLE_MATCH = re.compile(r'(.*) \(\d{1,10}.{1,100}\)$')
ADDRESS_MATCH = re.compile(r'.{1,100}\((.*), .{4,14}, United States\)$')
LOCATION_LISTING = re.compile(r'http://www\.locationary\.com/place/en/US/.{1,50}/.{1,50}/.{1,100}\.jsp')

def download(url):
    print "Downloading:", url
    s = urllib2.urlopen(url).read()
    if s[:2] == '\x1f\x8b': # assume it's gzipped data
        ifh = GzipFile(mode='rb', fileobj=StringIO(s))
        s = ifh.read()
    print "Downloaded: ", url
    return s

def replace_chars(text, replacements):
    return ''.join(replacements.get(x,x) for x in text)

def handle_listing(listing_url):
    listing_document = BeautifulSoup(download(listing_url))

    # ignore pages that link to yellowpages
    if not listing_document.find("a", href=re.compile(re.escape("http://www.yellowpages.com/") + ".*")):
        listing_title = listing_document.title.text
        reps = {' ':'-', ',':'', '\'':'', '[':'', ']':''}
        title, = TITLE_MATCH.match(listing_title).groups()
        address, = ADDRESS_MATCH.match(listing_title).groups()

        yellow_page_url = "http://www.yellowpages.com/%s/%s?order=distance" % (
            replace_chars(address, reps),
            replace_chars(title, reps),
        )

        yellow_page = BeautifulSoup(download(yellow_page_url))

        page_url = yellow_page.find("h3", {"class" : "business-name fn org"})
        if page_url:
            page_url = page_url.a["href"]

            business_name = title[:title.index(",")]

            page = BeautifulSoup(download(page_url))
            yellow_page_address =  page.find("span", {"class" : "street-address"})
            if yellow_page_address:

                if SequenceMatcher(None, address, yellow_page_address.text).ratio() >= 0.5:
                    pid, = re.search(r'p(\d{5,20})\.jsp', listing_url).groups(0)
                    page_escaped = replace_chars(page_url, {':':'%3A', '/':'%2F', '?':'%3F', '=':'%3D'})

                    final_url = "http://www.locationary.com/access/proxy.jsp?ACTION_TOKEN=proxy_jsp$JspView$SaveAction&inPlaceID=%s&xxx_c_1_f_987=%s" % (
                            pid, page_escaped)
                    return final_url


def main():
    pool = eventlet.GreenPool()
    listings_document = BeautifulSoup(download(START_URL))
    listings = listings_document.findAll("a", href = LOCATION_LISTING)
    listings = [listing['href'] for listing in listings]

    for final_url in pool.imap(handle_listing, listings):
        print final_url

if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ Thank you so much!! I especially like your humor! I'm new to this and I know I didn't originally post that but you still can't expect very good code from me...I just did this to automate using this website. Thanks again! \$\endgroup\$ – jacob501 Dec 24 '11 at 22:28
  • 1
    \$\begingroup\$ @jacob501, no worries. Making something this complex and having it works shows you've got something right! \$\endgroup\$ – Winston Ewert Dec 25 '11 at 6:31
1
\$\begingroup\$

I won't cover everything, but do you suppose this:

for z in range(0,len(ZA)):
    # ...
    time.sleep(5)

has anything to do with the code being slow? I didn't take the time to completely parse what in the world you are storing in a variable named ZA, but for each one in it you're going to sleep for 5 seconds. It seems to me that would slow the program down well more than almost any effect you'll get from actual processing.

\$\endgroup\$
  • 1
    \$\begingroup\$ That only happens in the last loop when it is launching web browsers. It is very slow in getting to that loop. \$\endgroup\$ – Winston Ewert Dec 27 '11 at 17:30

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.