[Tutor] Would an OOP approach be simpler?

Liam Clarke cyresse at gmail.com
Wed Oct 27 01:32:12 CEST 2004


Brilliant, thanks Danny, it's the structural stuff that I'm amiss on. 


On Sat, 23 Oct 2004 16:12:11 -0700 (PDT), Danny Yoo
<dyoo at hkn.eecs.berkeley.edu> wrote:
> 
> 
> On Sat, 23 Oct 2004, Liam Clarke wrote:
> 
> > Well thanks to all for their help, my first module of my first project
> > is completed.
> >
> > For anyone who's curious, the results are below -
> >
> > http://rafb.net/paste/results/qHyVC360.html
> >
> > Once again, thanks very much to all.
> 
> 
> Hi Liam,
> 
> Thanks for posting up your code!  I took a look at it; looks pretty good.
> Do you mind if I give a few suggestions?  If you do mind, um... skip this
> message.  *grin*
> 
> The only big target I can see so far is getReturns(): it's a bit long.
> It might be useful to break it up a little, by using helper functions to
> extract blocks of related statements.  For example, there's a block here:
> 
>     if os.path.exists('./archives/wb%s' % weekstart):
>         done=file('./archives/wb%s/got.lst' %weekstart, 'rb')
>         donelist=done.readlines()
>         done.close()
>         list=donelist[0].split('/')
>         doesExist=1
> 
> I'm not too familiar with what this is doing, but let's describe this
> block of code as something like checkIfDoesExist().  We can reform it as a
> function:
> 
> ###
> def checkIfDoesExist(weekstart):
>     if os.path.exists('./archives/wb%s' % weekstart):
>         done=file('./archives/wb%s/got.lst' %weekstart, 'rb')
>         donelist=done.readlines()
>         done.close()
>         list=donelist[0].split('/')
>         return (list, 1)
>     else:
>         return (None, 0)
> ###
> 
> and then, back in getReturns(), we can use it like this:
> 
>     (list, doesExist) = checkIfDoesExist(weekstart)
> 
> The reason this extraction ("refactoring") might be worthwhile is because
> the code doesn't make clear what value the 'list' should have if the
> directory doesn't exist.  The refactored code tries to make sure that
> 'list' does have some known value, regardless if 'doesExist' is true or
> not.
> 
> Another block in getReturns() that looks promising is the one that
> connects to the mail server.  This block here:
> 
>         success = 0
>         session = imaplib.IMAP4(hostname)
>         while not success:
>             try:
>                 session.login(user[account], pwd[account])
>             except:
>                 pass
>             else:
>                 success = 1
> 
> looks like it is making darn sure that it connects and logs into the
> server.  We can call this something like connectToImap():
> 
> ###
> def connectToImap(hostname, username, password):
>     success = 0
>     session = imaplib.IMAP4(hostname)
>     while not success:
>         try:
>             session.login(username, password)
>         except:
>             pass
>         else:
>             success = 1
>     return session
> ###
> 
> I made a few cosmetic changes here, mostly to reduce its direct ties to
> the user and pwd collections.  Back in getReturns(), we can replace the
> block with:
> 
>     session = connectToImap(hostname, user[account], pwd[account])
> 
> Again, this refactoring helps readability because it's clear that the
> block initializes a 'session' value.  The other variable, the 'success'
> variable, doesn't leak out into the rest of the getReturns() function.
> 
> 'success' is a temporary variable that's used only to get that while loop
> working.  By refactoring the block out into a separate connectToImap()
> function, we get to make its local role very clear to a reader of the
> code.
> 
> Are these kinds of structural changes useful?  They don't add anything
> technically new, but they do make the code easier to read.  With those two
> blocks extracted out, getReturns() now reads like:
> 
> ###
> def getReturns(hostname, user, pwd, searchstring, weekstart):
>     sender = []
>     msgdata = []
> 
>     (list, doesExist) = checkIfDoesExist(weekstart)
> 
>     for account in range(len(user)):
>         session = connectToImap(hostname, user[account], pwd[account])
>         session.select() ...
> ###
> 
> So my main advice is to look out for places where "paragraph" blocks of
> code tend to appear, and see if those blocks can be extracted out as real
> functions.
> 
> I hope this helps!
> 
> 


-- 
'There is only one basic human right, and that is to do as you damn well please.
And with it comes the only basic human duty, to take the consequences.


More information about the Tutor mailing list