[Tutor] Would an OOP approach be simpler?

[Danny Yoo]

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!