[Tutor] Review my code

[Chanakya Mitra]

Hi Steve,
>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.

Noted and sorry about that.

Someone also pointed out I should have said 'please' in my original email, so apologies if I came across as rude and demanding.

Thanks for the feedback and critique, I'll be reworking the script with all your points in mind.

Chan



-----Original Message-----
From: tutor-bounces+chanakya=optirisk-systems.com at python.org on behalf of Steven D'Aprano
Sent: Mon 26/09/2011 16:42
To: Tutor at python.org
Subject: Re: [Tutor] Review my code
 
Chanakya Mitra wrote:
>  
> 
> I wrote a short script that tries to check if an email address exists or
> not.





> 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
_______________________________________________
Tutor maillist  -  Tutor at python.org
To unsubscribe or change subscription options:
http://mail.python.org/mailman/listinfo/tutor

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/tutor/attachments/20110926/8723c070/attachment-0001.html>