Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

The text of a website is checked in a given time period. If there are any changes a mail is sent. There is a option to show/mail the new parts in the website. What could be improved?

#!/usr/bin/env python3
import urllib.request, hashlib, time, html2text, smtplib, datetime, argparse

class urlchange:
    def __init__(self, url):
        self.url = url
        self.urlhash = self.createhash()
        self.content = self.getcontent()
        date = datetime.datetime.now().strftime( "%d.%m.%Y %H:%M:%S" )
        print(date+": Start Monitoring... hash: "+self.urlhash)

    def getcontent(self):
        #Try to get data
        try:
            urldata = urllib.request.urlopen(self.url).read().decode("utf-8","ignore")
            urldata = html2text.html2text(urldata)
        except:
            print("Can't open url: ", self.url)
        return urldata

    def createhash(self):
        #create hash
        urldata = self.getcontent().encode("utf-8")
        md5hash = hashlib.md5()
        md5hash.update(urldata)
        return md5hash.hexdigest()

    def comparehash(self):
        date = datetime.datetime.now().strftime( "%d.%m.%Y %H:%M:%S" )
        if(self.createhash() == self.urlhash):
            print(date+": Nothing has changed")
            return False
        else:
            print(date+": Something has changed")
            if(not args.nodiff):
                print(self.diff())
                if(not args.nomail):
                    try:
                        sendmail("Url has changed!","The Url "+self.url+" has changed at "+date+" .\n\nNew content:\n"+self.diff())
                    except:
                        sendmail("Url has changed!","The Url "+self.url+" has changed at "+date+" .")
            elif(not args.nomail):
                sendmail("Url has changed!","The Url "+self.url+" has changed at "+date+" .")
            return True

    def diff(self):
        #what has chaged
        start, end = 0, 0
        newcontent = self.getcontent()
        #start of changes
        for i,j in enumerate(self.content):
            if(i<len(newcontent) and j != newcontent[i]):
                start=i
                break
        #end of changes
        for i,j in enumerate(reversed(self.content)):
            if( (len(newcontent)-(i+1))>0 and j != newcontent[len(newcontent)-(i+1)]):
                end=len(newcontent)-i
                break
        return newcontent[start:end]


def sendmail(subject,message):
    try:
        server = smtplib.SMTP("smtp.server.com",587)
        server.set_debuglevel(0)
        server.ehlo()
        server.starttls()
        server.login("email@server.de","password")
    except:
        print("Can't connect to the SMTP server!")

    date = datetime.datetime.now().strftime( "%d.%m.%Y %H:%M:%S" )
    msg = "From: email@server.de\nSubject: %s\nDate: %s\n\n%s""" % (subject, date, message)

    server.sendmail("email@server.de","email2@server.de",msg)
    server.quit()
    print(date+": email was sent")

parser = argparse.ArgumentParser(description="Monitor if a website has changed.")
parser.add_argument("url",help="url that should be monitored")
parser.add_argument("-t","--time",help="seconds between checks (default: 600)",default=600,type=int)
parser.add_argument("-nd","--nodiff",help="show no difference",action="store_true")
parser.add_argument("-n","--nomail",help="no email is sent",action="store_true")
args = parser.parse_args()

url1 = urlchange(args.url)
time.sleep(args.time)
while(True):
    if(url1.comparehash()):
        break
    time.sleep(args.time)

Improved code at: Email a notification when detecting changes on a website (improved)

share|improve this question
2  
Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. – Simon Forsberg Feb 14 at 15:46
    
Oh sorry. Thanks for the advice. I should have read this before. This makes totaly sense. – questionanswer Feb 15 at 7:31
up vote 5 down vote accepted

You do not need to find the differences manually, you can use difflib.SequenceMatcher:

I think this is what you need:

>>> a, b = "foobxr", "foobar"
>>> diffs = difflib.SequenceMatcher(None, a, b).get_matching_blocks()
>>> diffs
[Match(a=0, b=0, size=4), Match(a=5, b=5, size=1), Match(a=6, b=6, size=0)]
>>> max((a, b), key=len)[diffs[0].size : diffs[1].a]
'x'

Specific except

Specify what you want to except exactly. Bare except catches even typos!

Use a logger

You print a lot of info, a logger is more flexible and can be very easily redirected to a file.

.format

You can make your messages more readable:

For example:

    print(date+": Start Monitoring... hash: "+self.urlhash)

Becomes:

print("{date}: Start Monitoring... Hash: {self.urlhash}".format(**locals()))

Or the more standard:

print("{}: Start Monitoring... Hash: {}".format(date, self.urlhash))
share|improve this answer
2  
difflib, wow, never even heard of this. Thanks, nice tip! – janos Feb 14 at 12:49
    
Thanks that is very helpful! But there is a performance problem with difflib. How do I know what Errors I have to expect? When I connect to the SMTP-server many things can fail. – questionanswer Feb 14 at 14:34
    
@questionanswer You can except many errors by making a tuple, for performance I think you will not find problems, just avoid it in tight loops. – Caridorc Feb 14 at 14:37
1  
Instead of the last code, I think it looks better print("{}: Start Monitoring... Hash: {}".format(date, self.urlhash)). – cdonts Feb 14 at 19:28
    
@cdonts Added as an alternative. – Caridorc Feb 15 at 14:39

Aside from implementation, here are some suggestions regarding the scope:

  • You may want to consider that some webpages have parts that change on a daily basis. (For example the sidebar with "Trending Articles".) Although one way to combat that might be to access the mobile version of a page rather than the desktop version.

  • If there are small parts of a page which do regularly change, you could add a threshold option (for example "number of different lines") that can be increased by the caller, if they find a page is returning notifications for irrelevant changes.

If you find "clutter" is becoming an issue, you could consider using a third-party library to extract the main content of the webpage, and perform the comparison on that part only. There are various different projects that attempt to automate this.

share|improve this answer

This is not how you handle errors!

try:
    urldata = urllib.request.urlopen(self.url).read().decode("utf-8","ignore")
    urldata = html2text.html2text(urldata)
except:
    print("Can't open url: ", self.url)
return urldata

It's entirely possible that you never set any value to urldata, so what are you supposed to return in that case? As it is you're creating a NameError. Instead, you should return from your except. By default a function will return None, so you can now just ensure that when you call on the function, you're dealing with the possibility that you don't have any urldata.

Good error handling will either fall back on alternative methods to perform the necessary code, or gracefully inform the user what's happening with a good error message. ie.

except:
    raise ValueError("Can't open url: " + self.url)

The user now would still know the data was not read, and know what url wasn't read (perhaps the url is the source of the problem) but the program doesn't erroneously attempt to run regardless. Importantly, a user could now also use a try except with this, and catch a ValueError in cases where urls can't be read. When you just allow the code to continue, the error is erratic and unclear, not something that can be counted on.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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