[Tutor] Review my code

[Steven D'Aprano]

Chanakya Mitra wrote:
>  
> 
> I wrote a short script that tries to check if an email address exists or
> not.


Some brief comments:

When you send code by HTML email, you mess up the formatting of the 
code. Please don't do that in the future.


> def mailxch(hname):
>     mxhosts = DNS.mxlookup(hname)
>     return mxhosts

That can be re-written as:

def mailxch(hname):
     return DNS.mxlookup(hname)

but why bother calling the function? Instead of:

x = mailxch("some host name")

why not just call the actual function that does the work?

x = DNS.mxlookup("some host name")


If all you want is to use a different name, because you don't like 
"mxlookup", then:

mailxch = DNS.mxlookup

will bind the mxlookup function to the name "mailxch" without the cost 
of an extra function call.



> def prompt(prompt):
>     return raw_input(prompt).strip()

Another function which may be simple enough to just call inline. But 
that's a matter of taste.


> class smtsock(object):
>     response = None

This class has no initialiser (no __init__ method). Is that deliberate?

Also, it is normal for classes to be named in CamelCase.



>     def getmsg(self):
>         self.response = self.msock.makefile('rb')
>         while 1:

The more Pythonic idiom is to say "while True" rather than "while 1", 
although again that's a matter of taste.


>             try:
>                line = self.response.readline()
>                return line[:3], line[4:]               
>             except socket.error:
>                 self.msock.close()
>                 return "socket Error"

Generally speaking, this is poor design. Now you have a method which 
sometimes returns a tuple of lines, and sometimes a string. So you can't 
do this:

alines, blines = smtsock().getmsg()

because it will break if there is a socket error. So you have to do this:


result = smtsock().getmsg()
if result == "socket error":  # oops, I spelled it wrong...
     # how do I recover from this?
     pass  # ???
else:
     alines, blines = result


Ugly and fragile! Don't write code like this. Instead, errors should 
raise an exception. The caller can always catch the exception if they 
know how to deal with it, or they can ignore it and let it bubble up to 
the top, then they will get a nice traceback.

So something like this is probably better:

             try:
                 line = self.response.readline()
                 return line[:3], line[4:]
             finally:
                 # Always close the socket, even if an error occurred.
                 self.msock.close()


If an error occurs, you will get an exception, but the socket will still 
be closed.



>     def put(self,cmd,args=""):
>         if args == "":
>             str = '%s%s' % (cmd, CRLF)
>         else:
>             str = '%s %s%s' % (cmd, args, CRLF)        
>         try:
>             self.msock.send(str)
>         except socket.error:
>             self.msock.close()
>             return "socket Error"

Same here. Never return a magic error code when you can use an exception 
instead.


>     def pr(self,cmd,args=""):
>         self.put(cmd, args) 
>         return self.getmsg()
> 
>     def conn(self,server):
>         try:
>             self.msock = socket.create_connection((server,25))
>             (hand, msg) = self.getmsg()
>             return hand, msg
>         except:

Never catch a bare except! (Well, truth be told there are situations 
where catching a bare except is useful, but they're rare.) Bare excepts 
catch too much -- they mask bugs in your code, they prevent the user 
from interrupting the script with Ctrl-C. You should only ever catch the 
smallest set of exceptions that you know you can deal with.


>             err = sys.exc_info()[1]
>             return err

Again, don't return magic error codes.


>     def helo(self, server):             
>         (hand, msg) =  self.pr("HELO",server)
>         return hand, msg
> 
>     def vrfy(self, hname):
>         cstr =  "<%s>"% hname
>         (hand, msg) = self.pr("VRFY", cstr)
>         return hand, msg
> 
>     def mailfrm(self, emf):
>         cstr =  "<%s>"% emf        
>         (hand, msg) = self.pr("MAIL FROM:", cstr)
>         return hand, msg
> 
>     def rcptto(self,hname):
>         cstr =  "<%s>"% hname        
>         (hand, msg) = self.pr("rcpt to:", cstr)
>         return hand, msg
> 
>     def cls(self):
>         self.msock.close()

Apart from the painfully terse method names ("cls"? Would typing "close" 
break your keyboard?), nothing looks too bad there.





-- 
Steven