4
\$\begingroup\$

This is a Python document (PDF) downloader I made to download some question papers automatically. However, it is very slow. Any better way to do this?

try:
    import urllib2
    from bs4 import BeautifulSoup
    from time import sleep
    import os
    import sys
    from time import sleep
except ImportError:
    print "ImportError !"

url = raw_input('enter the url :')
c = 0

while True:
    path = raw_input("\nGive the directory Name where to save the files with full path :")
    if c == 3:
        if raw_input("you have tried 3 times do you want to create the folder ? (y/n)").lower().startswith('y'):
            os.mkdir(path)
            c = 0
            break
    else: 
        if os.path.isdir(path):
            break
        else:
            print "\nEnter Path Carefully !\n"
            c+=1
            continue


headers = {'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36'}

request = urllib2.Request(url,None,headers)
html = urllib2.urlopen(request)
soup = BeautifulSoup(html.read(), "html.parser")
soup.prettify()

#modifying the url for concatonation with 
url =  url.split('/') 
del url[-1]
url.append('')
url = '/'.join(url)
urls = []
names = []

for anchor in soup.findAll('a', href=True): #Going inside links
    req = urllib2.Request(anchor['href'],None,headers)
    pag = urllib2.urlopen(req)
    pgf = pag.read()
    soup1 = BeautifulSoup(pgf, "html.parser")

    for i, link in enumerate(soup1.findAll('a')): # Download all pdf inside each link
        fullurl = url + str(link.get('href'))
        #print fullurl

        if fullurl.endswith('.pdf'):
            #print fullurl
            urls.append(fullurl)
            names.append(soup1.select('a')[i].attrs['href'])

names_urls = zip(names, urls)
os.chdir(path)

for name, url1 in names_urls:
    try:
        if os.path.isfile(name):
            print name, "Already exists skipping..."
            pass
        else:
            rq = urllib2.Request(url1)
            res = urllib2.urlopen(rq)
            with open(name, 'wb') as pdf:
                pdf.write(res.read())
            print name, "Downloaded"
    except:
        print name, "Failed to download"
        pass
    sleep(1)
\$\endgroup\$
6
\$\begingroup\$

When you encounter an ImportError it does not really help you debug the problem if you ignore it, just print the string "Import Error" and then fail at some random later point where that module is actually needed. This will generate a lot more error messages which are less clear than the normal ImportError!


Think about using argparse for your command line arguments. This will not allow you the shenanigans of asking three times for an existing directory name before offering to create it, but you can just add a flag for this:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument("url", help="The base page where to search for PDF files.")
parser.add_argument("path", help="Where to save the files")
parser.add_argument("--create-directory", "-p", action="store_true", help="Create the directory if it does not exist")
locals().update(vars(parser.parse_args()))
if not os.path.isdir(path):
    if create_directory:
        os.makedirs(path)
    else:
        raise OSError("Directory {} does not exist. Use the '-p' flag to create it".format(path))

I used a trick here to update the locals dictionary, which is just the variables in the local scope. This way the variable url and path exist and have the right value. This magic works only outside of a function, inside you can use:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument("url", help="The base page where to search for PDF files.")
parser.add_argument("path", help="Where to save the files")
parser.add_argument("--create-directory", "-p", action="store_true", help="Create the directory if it does not exist")
args = parser.parse_args()
if not os.path.isdir(args.path):
    if create_directory:
        os.makedirs(args.path)
    else:
        raise OSError("Directory {} does not exist. Use the '-p' flag to create it".format(args.path))

Use this argument parser by calling your script like:

$ python pdf_downloader.py http://url.to/pdfs.html path/to/save/files/to/

If you don't enter the two required arguments, an automatically generated usage message will be displayed:

$ python pdf_download.py
usage: pdf_download.py [-h] [-p] url path
pdf_download.py: error: too few arguments

$ python pdf_download.py --help
usage: pdf_download.py [-h] [-p] url path

positional arguments:
  url         The base page where to search for PDF files.
  path        Where to save the files

optional arguments:
  -h, --help              show this help message and exit
  -p, --create-directory  Create the directory if it does not exist

Interlude: Python has an official style guide, PEP8. One of the things it recommends is using a blank after a comma in a(n) (argument-)list, like (1, 2, 3). Another thing is to use lower_case for variable and function names. In all of the below code I fixed this.


Your url manipulation is probably better written as

base_url = '/'.join(url.split('/')[:-1])

And the separating trailing '/' can be added when actually building the full url. I would also put it into a new variable, just to make it clearer what this is.

Even better would be to just use the built-in os.path.dirname, since you already imported os:

base_url = os.path.dirname(url)

For the full url I would use str.format, which makes it a bit easier to read and which is also faster than multiple string additions (even though for one addition it does not matter).

full_url = "{}/{}".format(base_url, link.get('href'))

Instead of separately saving the urls to download and their names, only to zip it first and then iterate over the zipped container, just save the two together right away:

    if full_url.endswith('.pdf'):
        # print fullurl
        name = soup1.select('a')[i].attrs['href']
        urls.append((name, full_url))

pass is not what you think it is. It is a no-op that does nothing (you can use it as placeholder when you need some code block there because of indentation but don't have the code yet). What you mean is continue which directly goes to the next iteration of the loop.


You should never have a bare except clause. This will also catch e.g. CTRL-C if you run into an infinite loop and want to abort it! Use at least except Exception or better, figure out the actual exceptions you code would raise.

Also try to limit the scope of the except to the one call that might fail, if possible. At leas use except Exception as e and print the exception afterwards. Otherwise this information is lost.


Figuring out which urls to download and actually downloading them are two distinct tasks, which are ideal for functions. In addition I would add a main function for the rest and guard it with a if __name__ == "__main__": clause. The latter allows you to do in another script to just do from pdf_download import get_urls and reuse that function.

This results in the final code:

import urllib2
from bs4 import BeautifulSoup
from time import sleep
import os
import sys
import argparse

HEADERS = {'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36'}


def get_urls(url):
    request = urllib2.Request(url, None, HEADERS)
    soup = BeautifulSoup(urllib2.urlopen(request).read(), "html.parser")
    soup.prettify()

    urls = []
    base_url = os.path.dirname(url)
    for anchor in soup.findAll('a', href=True): #Going inside links
        req = urllib2.Request(anchor['href'], None, HEADERS)
        soup1 = BeautifulSoup(urllib2.urlopen(req).read(), "html.parser")

        for i, link in enumerate(soup1.findAll('a')): # Download all pdf inside each link
            full_url = "{}/{}".format(base_url, link.get('href'))
            if full_url.endswith('.pdf'):
                name = soup1.select('a')[i].attrs['href']
                urls.append((name, full_url))
    return urls


def download(urls, path):
    old_dir = os.getcwd()
    os.chdir(path)
    for name, url in urls:
        if os.path.isfile(name):
            print name, "already exists, skipping..."
            continue
        try:
            request = urllib2.Request(url)
            res = urllib2.urlopen(request).read()
            with open(name, 'wb') as pdf:
                pdf.write(res)
            print "Downloaded", name
        except Exception as e:
            print "Failed to download", name, ", because of", e
    os.chdir(old_dir)


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument("url", help="The base page where to search for PDF files.")
    parser.add_argument("path", help="Where to save the files")
    parser.add_argument("--create-directory", "-p", action="store_true", help="Create the directory if it does not exist")
    args = parser.parse_args()

    if not os.path.isdir(args.path):
        if args.create_directory:
            os.makedirs(args.path)
        else:
            raise OSError("Directory {} does not exist. Use the '-p' flag to create it".format(args.path))

    download(get_urls(args.url), args.path)

if __name__ == "__main__":
    main()

Here I made some more cosmetic changes, like using proper grammar in the notifications ("Downloaded file_name" instead of "file_name Downloaded"), reverting back to the original working directory after downloading all files, removing unnecessary comments and made headers a constant, because it is used in multiple functions now. I also deleted the sleep because it is usually not necessary (unless there is some flood protection on the website, maybe).

\$\endgroup\$
  • \$\begingroup\$ awesome , had a little syntax error urls.append((name, full_url)) it need to be tuple and --create-directory was typed differently everything is AWESOME otherwise \$\endgroup\$ – Shantanu Bedajna Aug 26 '16 at 19:08
  • \$\begingroup\$ i just didn't understood this method base_url = os.path.dirname(url) how can a os method strip the html part of the url and only return the dir part ? \$\endgroup\$ – Shantanu Bedajna Aug 26 '16 at 19:09
  • \$\begingroup\$ for some reason it was complaining that "path" is not defined i removed the "-" from the optional argument and everything was fine \$\endgroup\$ – Shantanu Bedajna Aug 28 '16 at 14:08
  • \$\begingroup\$ @ShantanuBedajna Found out what the problem was. The overriding the locals() is not guaranteed to work inside a function (and it doesn't here). I reverted it back to using the namespace. \$\endgroup\$ – Graipher Aug 29 '16 at 8:02
  • \$\begingroup\$ @ShantanuBedajna BTW, personally, I just use wget for this task: wget -A pdf -m -p -E -k -K -np -nd URL will download all linked pdf from URL to the current directory. \$\endgroup\$ – Graipher Aug 29 '16 at 10:43

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.