6
\$\begingroup\$

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 - follow-up

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Simon Forsberg Feb 14 '16 at 15:46
  • \$\begingroup\$ Oh sorry. Thanks for the advice. I should have read this before. This makes totaly sense. \$\endgroup\$ – questionanswer Feb 15 '16 at 7:31
  • \$\begingroup\$ You could comment more and more precise , so that your code has more structure. I've got one question for you.. I am a beginner in programming and your code seems really interesting. I would like to work with it a bit (like changing and adding some features) but like I said, I am not the best. Would you be so nice and could you explain to me how your code works, why you did it like that and not used some other method, and so on. Would be really nice. You don't have to, of course. Thank you! \$\endgroup\$ – Lucas Mar 10 '17 at 18:25
6
\$\begingroup\$

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))

Thanks to @Jatimir for noticing that in python 3.6+ f-strings are a nice way to interpolate variables in strings with a clean sintax: for example:

print(f'{date}: Start Monitoring... Hash: {self.urlhash}')
\$\endgroup\$
  • 3
    \$\begingroup\$ difflib, wow, never even heard of this. Thanks, nice tip! \$\endgroup\$ – Stop ongoing harm to Monica Feb 14 '16 at 12:49
  • \$\begingroup\$ 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. \$\endgroup\$ – questionanswer Feb 14 '16 at 14:34
  • \$\begingroup\$ @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. \$\endgroup\$ – Caridorc Feb 14 '16 at 14:37
  • 1
    \$\begingroup\$ Instead of the last code, I think it looks better print("{}: Start Monitoring... Hash: {}".format(date, self.urlhash)). \$\endgroup\$ – cdonts Feb 14 '16 at 19:28
  • \$\begingroup\$ @cdonts Added as an alternative. \$\endgroup\$ – Caridorc Feb 15 '16 at 14:39
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\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.