5
\$\begingroup\$

This code goes to the website containing the PDF, downloads the PDF, then it converts this PDF to text. Finally, it reads this whole file (Over 5000 lines) into a list, line by line, and searches for my name in it.

import mechanize
import os
br = mechanize.Browser()
br.open("http://sttm.org/Bulletin/Bulletins2015/tabid/483/LinkClick.aspx?fileticket=WkjzL8SuMQQ%3d&tabid=483&portalid=0&mid=979")
print br.title()
pdfs = []
for i in br.links():
    if "Sunday," in i.text:
        pdfs.append("http://sttm.org/Bulletin/Buletins2015/" + i.url)
br.retrieve(pdfs[0], "file.pdf")
input = "file.pdf"
output = "out.txt"
os.system(("ps2ascii %s %s") %(input, output))
with open("out.txt") as f:
    list = f.readlines()
scheduled = False
for i in list:
    if "My Name Here" in i:
        scheduled = True;
if scheduled == True:
    print "Yes"
else:
    print "No"

It's a heck of a lot of code to complete this simple task. Also, as it is taking in 5000 lines and reading them one by one, it takes quite a long time to run this.

\$\endgroup\$
  • \$\begingroup\$ Wow, that website has some nasty nested tables! \$\endgroup\$ – 200_success Jan 26 '15 at 22:52
3
\$\begingroup\$
  • Avoid mingling configurable strings, such as the initial URL, with the body of the code.
  • Make use of the support in mechanize to follow links, instead of constructing your own URL to open based on the <a href="…">. In fact, you shouldn't hard-code that nasty long initial URL. A human would start at the front page and follow the "Bulletins 2015" link in the navigation menu; your script should do that too.
  • Avoid writing to temporary files, and use Popen instead.
  • You can often avoid writing explicit loops in Python, for example, by using any().
  • Consider using a Python PDF library, such as slate, instead of piping to an external process.

I've opted to keep it simple and avoid gracefully handling the failure to fetch the PDF. If the script fails to download the PDF, I'd rather have it crash than give you the impression that you are not scheduled to appear in a given week.

import mechanize
from subprocess import Popen, PIPE

INITIAL_URL = "http://sttm.org/"

br = mechanize.Browser()
br.open(INITIAL_URL)
br.follow_link(text='Bulletins 2015')
print br.title()
pdf_req = br.follow_link(text_regex=r'Sunday,')

ps2ascii = Popen(['ps2ascii'], stdin=PIPE, stdout=PIPE, shell=True)
ps2ascii.stdin.write(pdf_req.read())
ps2ascii.stdin.close()
scheduled = any(text for text in ps2ascii.stdout if "My Name Here" in text)
ps2ascii.stdout.close()
ps2ascii.wait()
print "Yes" if scheduled else "No"
\$\endgroup\$
4
\$\begingroup\$

First of all standard library imports and third-party library imports should be kept separate:

from subprocess import PIPE, Popen
from urlparse import urljoin

import mechanize

It's better to put your code in a function so that you re-use it with different urls, search keywords etc. Further explanation in comments:

def pdf_contains(url, file_url, search_key, keyword=''):
    br = mechanize.Browser()
    br.open(url)
    print br.title()
    # As you're interested only in the first link text that contains
    # the keyword('Sunday,' in this case) we should better use next()
    # with a generator expression. `next()` will yield the first item from
    # the generator if any otherwise we'll return None
    # Also `urlparse.urljoin` can come handy in joining urls.

    pdf = next((urljoin(file_url, f.url) for f in br.links()
               if keyword in f.text), None)

    # Now instead of downloading the file using .urlretrive we 
    # can simply simply get a file-like object using
    # `mechanize.urlopen` which then we can then pass to
    # subprocess's STDIN 

    if pdf is not None:
        data = mechanize.urlopen(pdf)

        # Now instead of running `os.system` and storing the output
        # in a file we can use `subprocess.Popen` to store the
        # output of ps2ascii command in PIPE

        proc = Popen(['ps2ascii'], stdin=data, stdout=PIPE)

        # Now simply read the data line by line from PIPE
        # and check for the search_key, if found return instantly 
        for line in iter(proc.stdout.readline, ''):
            if search_key in line:
                return True
    return False

url = ("http://sttm.org/Bulletin/Bulletins2015/tabid/483/Link"
       "Click.aspx?fileticket=WkjzL8SuMQQ%3d&tabid=483&portal"
       "id=0&mid=979", "http://sttm.org/Bulletin/Buletins2015/")

scheduled = pdf_contains(url,
                         "http://sttm.org/Bulletin/Buletins2015/",
                         "My Name Here",
                         "Sunday,")

print scheduled
\$\endgroup\$
3
\$\begingroup\$

You should structure your code into functions, not have everything in the top-level scope.

You should wrap long lines, even if they are only strings.

Don't do

if scheduled == True

Instead, do if scheduled.

Don't call variables list; you shadow the builtin and don't adequately name the variable anyway.

Instead of

scheduled = False
for i in list:
    if "My Name Here" in i:
        scheduled = True;

do

scheduled = any("My Name Here" in i for i in list)

Don't use f.readlines(); try list(f) or just use f directly.

Names like i and f are terrible.

Don't use os.system(("ps2ascii %s % s") % (input, output)); not only should you be using new-style formatting and escaping, and removing the redundant parenthesis,

os.system("ps2ascii {} {}".format(
    shlex.quote(input),
    shlex.quote(output)
)

but you should be using subprocess anyway. Further, you should be reading from its output, not directing through a file:

subprocess.call(['ps2ascii', input, output])

That said, Ashwini Chaudhary points out that you can just make a straightforward pipeling:

data = mechanize.urlopen(pdfs[0])
proc = Popen(['ps2ascii'], stdin=data, stdout=PIPE)

You want the first pdf in the text, so you can either also use Ashwini Chaudhary's method or something like

for i in br.links():
    if link_filter in i.text:
        break
else:
    return False

or

try:
    i = next(i for i in br.links() if link_filter in i.text)
except StopIteration:
    return False

You don't seem to be using mechanize's fancy features. It seems simpler just to use requests and beautifulsoup. This will also give you the ability to support the much better Python 3.

Now, you also need to keep into account, it seems, the newlines that are embedded in the text.

Now, finally, one should use with on Popen objects on Python 3. If you want to stick to Python 2, consider closing the subprocess. Albeit this isn't a big deal; the shell itself isn't so fussy!

Here's the result:

import os
import re
from subprocess import PIPE, Popen
from urllib.parse import urljoin

import requests
from bs4 import BeautifulSoup

def pdf_contains(url, name, link_text_filter):
    page = requests.get(url)
    parsed = BeautifulSoup(page.content)
    print(parsed.title.text.strip())

    link = parsed.find("a", text=link_text_filter)
    if link is None:
        return False
    data = requests.get(urljoin(url, link.get("href")))

    with Popen(['ps2ascii'], stdin=PIPE, stdout=PIPE) as proc:
        text, err = proc.communicate(data.content)
    return name in text.replace(b'\n', b'')

def main():
    url = (
        "http://sttm.org/Bulletin/Bulletins2015/tabid/483/LinkClick.aspx"
        "?fileticket=WkjzL8SuMQQ%3d&tabid=483&portalid=0&mid=979"
    )

    found = pdf_contains(url, b"My Name Here", re.compile(".*Sunday,.*"))
    print("Yes" if found else "No")

if __name__ == '__main__':
    main()
\$\endgroup\$

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.