2
\$\begingroup\$

I need to write some code that checks thousands of websites, to determine if they are in English or not. Below is the source code. Any improvements would be appreciated.

import nltk
import urllib2
import re
import unicodedata

ENGLISH_STOPWORDS = set(nltk.corpus.stopwords.words('english'))
NON_ENGLISH_STOPWORDS = set(nltk.corpus.stopwords.words()) - ENGLISH_STOPWORDS

STOPWORDS_DICT = {lang: set(nltk.corpus.stopwords.words(lang)) for lang in nltk.corpus.stopwords.fileids()}

def get_language(text):
    words = set(nltk.wordpunct_tokenize(text.lower()))
    return max(((lang, len(words & stopwords)) for lang, stopwords in STOPWORDS_DICT.items()), key=lambda x: x[1])[0]


def checkEnglish(text):
    if text is None:
        return 0
    else:
        text = unicode(text, errors='replace')
        text = unicodedata.normalize('NFKD', text).encode('ascii', 'ignore')
        text = text.lower()
    words = set(nltk.wordpunct_tokenize(text))
    if len(words & ENGLISH_STOPWORDS) > len(words & NON_ENGLISH_STOPWORDS):
        return 1
    else:
        return 0


def getPage(url):
    if not url.startswith("http://"):
        url = "http://" + url
    print "Checking the site ", url
    req = urllib2.Request(url)
    try:
        response = urllib2.urlopen(req)
        rstPage = response.read()
    except urllib2.HTTPError, e:
        rstPage = None
    except urllib2.URLError, e:
        rstPage = None
    except Exception, e:
        rstPage = None
    return rstPage


def getPtag(webPage):
    if webPage is None:
        return None
    else:
        rst = re.search(r'<p\W*(.+)\W*</p>', webPage)
        if rst is not None:
            return rst.group(1)
        else:
            return rst


def getDescription(webPage):
    if webPage is None:
        return None
    else:
        des = re.search(r'<meta\s+.+\"[Dd]escription\"\s+content=\"(.+)\"\s*/*>', webPage)
        if des is not None:
            return des.group(1)
        else:
            return des


def checking(url):
    pageText = getPage(url)
    if pageText is not None:
        if checkEnglish(getDescription(pageText)) == 1:
            return '1'
        elif checkEnglish(getPtag(pageText)) == 1:
            return '1'
        elif checkEnglish(pageText) == 1:
            return '1'
        else:
            return '0'
    else:
        return 'NULL'

if __name__ == "__main__":
    f = open('sample_domain_list.txt').readlines()
    s = open('newestResult.txt', "w")
    for line in f[:20]:
        url = line.split(',')[1][1:-1]
        check = checking(url)
        s.write(url + ',' + check)
        s.write('\n')
        print check

#    f.close()
    s.close()
\$\endgroup\$
1
  • \$\begingroup\$ Does your code work as you intend it to? What problems do you see with it? (To help us focus on those...) \$\endgroup\$
    – mac389
    Aug 16, 2012 at 13:57

2 Answers 2

3
\$\begingroup\$

Some of your functions behave a bit unconventionally.

checkEnglish() only returns 1 or 0. It would be clearer to return True or False, and rename the function to isEnglish().


getPage() won't allow retrieval of HTTPS URLs. I would avoid trying to be "helpful" by automatically mangling the URL, unless such mangling was really smart and thorough (as good as the intelligence in your browser's address bar). By the way, URI schemes are case insensitive (RFC 3986 Sec 3.1).

In getPage(), you swallow exceptions. That's not good practice, but if you're going to do it, do it succinctly:

def getPage(url):
    print "Checking the site ", url
    req = urllib2.Request(url)
    try:
        response = urllib2.urlopen(req)
        return response.read()
    except:
        return None

In getPtag() and getDescription(), avoid nesting:

def getPtag(webPage):
    if webPage is None:
        return None
    match = re.search(r'<p\W*(.+)\W*</p>', webPage)
    if not match:
        return None
    return match.group(1)

In general, HTML is case insensitive, so use case-insensitive regular expression matching (or an HTML parser).


Your checking() function could use some improvement:

  • Returning strings '1', '0', or 'NULL' is really weird. Returning True, False, or None would make more sense.

  • The name of the function is weird. It should be something like isEnglishUrl().

  • Prefer to return early, and express the cascade more simply.

      def isEnglishUrl(url):
          pageText = getPage(url)
          if pageText is None:
              return None
          return isEnglish(getDescription(pageText)) or \
                 isEnglish(getPtag(pageText)) or \
                 isEnglish(pageText)
    

    If getPage() hadn't swallowed exceptions in the first place, then isEnglishUrl() wouldn't have to deal with that pesky if pageText is None. Instead, it could just let the exception propagate and let its caller deal with it, for more flexibility with less code.


Since Python 2.5, the preferred way to open and close files is using a with block:

RESULT_STR = { True: '1', False: '0', None: 'NULL' }
with open('sample_domain_list.txt') as f:
    with open('newestResult.txt', 'w') as s:
        for line in f.readlines()[:20]:
            url = line.split(',')[1][1:-1]
            eng = isEnglishUrl(url)
            s.write("%s,%s\n" % (url, RESULT_STR[eng]))
\$\endgroup\$
0
\$\begingroup\$
  1. Use BeautifulSoup to strip the JS, HTML, and CSS formatting.
  2. Use urllib instead of urllib2.

from bs4 import BeautifulSoup
from urllib import urlopen
url = "http://stackoverflow.com/help/on-topic"

def getPage(url)     
 html = urlopen(url).read()
 soup = BeautifulSoup(html)

# remove all script and style elements
 for script in soup(["script", "style"]):
    script.extract()    # remove

# get text
  text = soup.get_text()
  return text
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.