Minimal webcrawler - bad structure and error handling?

Question

I did this code over one day as a part of a job application, where they wanted me to make a minimal webcrawler in any language. The purpose was to crawl a site, find all of the URLs on that page, and crawl on to other pages. The code had to be able to resume operation if shut down (that's why I saved the lists to text files). I made it in Python just for simplicity, although I have never made anything more than Project Euler stuff in Python. Commented it excessively to make it highly readable.

I did not get the job, but I am interested in getting some input on how I can make the crawler better, and if there are some major flaws/structural errors. It was a fun task.

import urllib
import re
import os

# The parameter is the url the crawler is gonna crawl.
def crawler(url):
    # The crawler find links by utilizing pythons urllib and the href tag
    for new_url in re.findall('''href=["'](.[^"']+)["']''', urllib.urlopen(url).read()): 
        new_url = re.sub('\/*$', '', new_url)

    # A bit ugly, but this is to be sure that the new url is not in the urls already crawled, or scheduled for crawling, a check for if it's a HTML element that have escape the regex, or a "link" to a html class on the same page, and that the url ending is in the allowed list
        if not any(new_url in word for word in urls_Crawled) and not any(new_url in word for word in urlsToCrawl_Parent)and not any(new_url in word for word in urlsToCrawl_Child) and '<' not in new_url and '#' not in new_url and any(word in new_url.split('.')[-1] for word in allowedList) and 'https://www' not in new_url and 'mailto' not in new_url:

        # If the url is in the new url, f.ex http://www.telenor.com is inside http://www.telenor.com/no/personvern/feed
        # the latter should be appended in the parent list as i prioritize the local urls first.
            if url.replace("http://","").replace("www.","").split('/', 1)[0] in new_url:
                urlsToCrawl_Parent.append(new_url)

        # Another check, this is if we deal with a local link on the form /no/personvern/feed/
            elif new_url.startswith('/'):
            # To better store local links, f.ex /no/personvern/feed/ forward slash (/) is omitted
                new_url = re.sub('\/*$', '', new_url)
                url = re.sub('\/*$', '', url)

            # The /no/personvern/feed/ url is stored as www.telenor.com/no/personvern/feed/
                urlsToCrawl_Parent.append(url+new_url)

        # If we are not dealing with an local URL we are dealing with a child URL 
            else:
                urlsToCrawl_Child.append(new_url)

# We are done crawling this URL and thus we remove it from the list, and we add it to the list over urls crawled.
        urlsToCrawl_Parent.pop(0)
        urls_Crawled.append(url)

# A fast implemention of a "blind" saving of state so the program can be resumed after an error
    writetofile(urls_Crawled, 'URLsCrawled')
    writetofile(urlsToCrawl_Parent, 'parentURLs')
    writetofile(urlsToCrawl_Child, 'childURLs')

# Here we write the list over urls crawled to a txt file
def writetofile(urls, filename):

    with open(filename,'w') as file:
        for item in urls:
            print>>file, item

if __name__ == "__main__":

# This is the 'input' parameteres the max_url gives how many different url it's gonna crawl. 
# this is implemented to give a better control of the runtime of this crawler.
        starturl = ""
    max_urls = 500

    if not os.path.exists('parentURLs') or not os.path.getsize('parentURLs') > 0 or not os.path.exists('childURLs') or not os.path.getsize('childURLs') > 0:

    # This is simple lists to hold the urls crawled, the parent urls, i.e the 'local' urls on the current page, while the 
    # child urls are urls for other web pages found on a parent page.
        urls_Crawled = []
        urlsToCrawl_Parent = []
        urlsToCrawl_Child = []

    # We start to append the starturl
        urlsToCrawl_Parent.append(starturl)

    else:
            with open('URLsCrawled', 'r') as f:
            urls_Crawled = [line.rstrip('\n') for line in f]
        with open('parentURLs', 'r') as f:
            urlsToCrawl_Parent = [line.rstrip('\n') for line in f]
        with open('childURLs', 'r') as f:
            urlsToCrawl_Child = [line.rstrip('\n') for line in f]

# To avoid crawl links to pictures (.jpeg) and other files i have made an list over allowed url-endings.
    allowedList = ['html', 'htm', 'php', 'jsp', 'jspx', 'asp', 'no', 'com', 'net', 'org', 'se', 'dk']

# A while loop is utilized so we crawl the next url in the list until we have sucessfully crawled the amount of 
# URLs wanted, or until we have reached the depth specified.
    while (len(urls_Crawled) < max_urls):

    # A while loop is utilized to always crawl the parent URLs first
        while urlsToCrawl_Parent and (len(urls_Crawled) < max_urls):

        # An extra check if the url is already crawled, if not we crawl this url
            if urlsToCrawl_Parent[0] in urls_Crawled:
                urlsToCrawl_Parent.pop(0)
            elif ('http://' in urlsToCrawl_Parent[0] or 'https://' in urlsToCrawl_Parent[0]):
                    try:
                        crawler(urlsToCrawl_Parent[0])
                    except:
                        urlsToCrawl_Parent.pop(0) 
            else:
                urlsToCrawl_Parent[0] = "http://"+urlsToCrawl_Parent[0]
                try:
                    crawler(urlsToCrawl_Parent[0])
                except:
                    urlsToCrawl_Parent.pop(0)

    # When we are done with all the URLs in the parent list, we add the first URL in the children list
        urlsToCrawl_Parent.append(urlsToCrawl_Child[0])
        urlsToCrawl_Child.pop(0)

    os.remove('childURLs')
    os.remove('parentURLs')

Answer 1

import urllib
import re
import os

# The parameter is the url the crawler is gonna crawl.

That's a pretty useless comment. The parameter is named url. So I already know it is the URL.

def crawler(url):

Seeing as this is a function, it should really be named as a verb. The function is an action, not a thing.

    # The crawler find links by utilizing pythons urllib and the href tag
    for new_url in re.findall('''href=["'](.[^"']+)["']''', urllib.urlopen(url).read()): 

Any sort of regex being applied to extracting data from a webpage is suspicious. Consider using Beautiful soup or simliar to properly parse the HTML.

        new_url = re.sub('\/*$', '', new_url)

    # A bit ugly, but this is to be sure that the new url is not in the urls already crawled, or scheduled for crawling, a check for if it's a HTML element that have escape the regex, or a "link" to a html class on the same page, and that the url ending is in the allowed list
        if not any(new_url in word for word in urls_Crawled) 

Why are you checking if the new url is a substring?

  and not any(new_url in word for word in urlsToCrawl_Parent)
  and not any(new_url in word for word in urlsToCrawl_Child)

Instead of checking all these places, have a single set that you add to for each instance. Then you don't need to check three times.

  and '<' not in new_url and '#' not in new_url 

# at least is used in various valid urls.

  and any(word in new_url.split('.')[-1] for word in allowedList)

Why are checking against any possible word in the URL?

  and 'https://www' not in new_url 

What do you have against https? Why only https://www?

  and 'mailto' not in new_url:

What if you had "mailto" somewhere else in the URL?

        # If the url is in the new url, f.ex http://www.telenor.com is inside http://www.telenor.com/no/personvern/feed
        # the latter should be appended in the parent list as i prioritize the local urls first.
            if url.replace("http://","").replace("www.","").split('/', 1)[0] in new_url:
                urlsToCrawl_Parent.append(new_url)

Your code and comment don't really seem to be align. You seem to be checking whether it is on the same domain.

        # Another check, this is if we deal with a local link on the form /no/personvern/feed/
            elif new_url.startswith('/'):

You already stripped off the '/' so I don't see how this could happen.

            # To better store local links, f.ex /no/personvern/feed/ forward slash (/) is omitted
                new_url = re.sub('\/*$', '', new_url)
                url = re.sub('\/*$', '', url)

Modifying the URL here is dangerous. It makes it difficult to predict how it will work in the rest of the application.

            # The /no/personvern/feed/ url is stored as www.telenor.com/no/personvern/feed/
                urlsToCrawl_Parent.append(url+new_url)

        # If we are not dealing with an local URL we are dealing with a child URL 
            else:
                urlsToCrawl_Child.append(new_url)

# We are done crawling this URL and thus we remove it from the list, and we add it to the list over urls crawled.
        urlsToCrawl_Parent.pop(0)
        urls_Crawled.append(url)

# A fast implemention of a "blind" saving of state so the program can be resumed after an error
    writetofile(urls_Crawled, 'URLsCrawled')
    writetofile(urlsToCrawl_Parent, 'parentURLs')
    writetofile(urlsToCrawl_Child, 'childURLs')

Writing three different files seems excessive.

# Here we write the list over urls crawled to a txt file
def writetofile(urls, filename):

    with open(filename,'w') as file:
        for item in urls:
            print>>file, item

if __name__ == "__main__":

# This is the 'input' parameteres the max_url gives how many different url it's gonna crawl. 
# this is implemented to give a better control of the runtime of this crawler.
        starturl = ""
    max_urls = 500

    if not os.path.exists('parentURLs') or not os.path.getsize('parentURLs') > 0 or not os.path.exists('childURLs') or not os.path.getsize('childURLs') > 0:

It probably doesn't make sense to default to picking up, because they user might not realize that they left those files around. Instead, I'd suggest a command line --recover flag or something.

    # This is simple lists to hold the urls crawled, the parent urls, i.e the 'local' urls on the current page, while the 
    # child urls are urls for other web pages found on a parent page.
        urls_Crawled = []
        urlsToCrawl_Parent = []
        urlsToCrawl_Child = []

Don't store the program state in global variables. At least move this into a class.

    # We start to append the starturl
        urlsToCrawl_Parent.append(starturl)

    else:
            with open('URLsCrawled', 'r') as f:
            urls_Crawled = [line.rstrip('\n') for line in f]
        with open('parentURLs', 'r') as f:
            urlsToCrawl_Parent = [line.rstrip('\n') for line in f]
        with open('childURLs', 'r') as f:
            urlsToCrawl_Child = [line.rstrip('\n') for line in f]

# To avoid crawl links to pictures (.jpeg) and other files i have made an list over allowed url-endings.
    allowedList = ['html', 'htm', 'php', 'jsp', 'jspx', 'asp', 'no', 'com', 'net', 'org', 'se', 'dk']

# A while loop is utilized so we crawl the next url in the list until we have sucessfully crawled the amount of 
# URLs wanted, or until we have reached the depth specified.
    while (len(urls_Crawled) < max_urls):

    # A while loop is utilized to always crawl the parent URLs first
        while urlsToCrawl_Parent and (len(urls_Crawled) < max_urls):

        # An extra check if the url is already crawled, if not we crawl this url
            if urlsToCrawl_Parent[0] in urls_Crawled:
                urlsToCrawl_Parent.pop(0)

Having to check this twice is a sign of problematic code.

            elif ('http://' in urlsToCrawl_Parent[0] or 'https://' in urlsToCrawl_Parent[0]):
                    try:
                        crawler(urlsToCrawl_Parent[0])
                    except:
                        urlsToCrawl_Parent.pop(0) 

Don't ever except:. You don't know what exception happened or whether it was a real problem or a bug in your code.

            else:
                urlsToCrawl_Parent[0] = "http://"+urlsToCrawl_Parent[0]
                try:
                    crawler(urlsToCrawl_Parent[0])
                except:
                    urlsToCrawl_Parent.pop(0)

    # When we are done with all the URLs in the parent list, we add the first URL in the children list
        urlsToCrawl_Parent.append(urlsToCrawl_Child[0])
        urlsToCrawl_Child.pop(0)

    os.remove('childURLs')
    os.remove('parentURLs')

What about URLSCrawled?

Your code looks like you tried to solve the problem, and added a series of hacks on top of it to make it work without consideration towards whether or not that was really the correct solution. You just kept piling stuff onto the code and now it's a mess.

Your code also just doesn't show the structure of the problem very well. Ideally, your code would look something like:

def crawl(url):
    page = fetch_webpage(url)
    links = fetch_links(page)
    for link in links:
        if havent_visited(link):
              schedule_link(link)

That's a short function, and I can easily get the gist of what its doing. The details will be contained in the functions it calls.

Answer 2

My python isn't nearly good enough to verify this from the code , but checking from the comments in the code, I see 2 basic errors with the logic: outbound URLs seem to be parsed as well, and you are ignoring the robots.txt file.

The first error means that you are basically crawling the WHOLE web and not just that website. If example.com links to example2.com, it will also start parsing example2.com.

The second error means that you might crawl unintended pages, not to mention that you're more likely to be blacklisted as a crawler.

Answer 3

An alternative solution is to use Ruby (instead of Python) and take advantage of Charlie Somerville's Futures Library...

require "future"
require "open-uri"

def fetch_url(url)
  Future { open(url).read }
end

def crawl_urls(urls)
  Future.all(urls.map { |url|
    fetch_url(url)
  })
end

results = crawl_urls([
  "https://google.com/",
  "https://twitter.com/",
  "https://github.com/",
]).value