[Tutor] Program review

Jeff Younker jeff at drinktomi.com
Sat Jan 5 23:18:20 CET 2008


The procesar function is complicated.   It tries to do several things
that that are better done elsewhere.  It needs to be broken up into
several functions and methods.   This also clarifies and isolates the
exception handling.

> def procesar(mensaje):
>    try :
The enclosing try block isn't needed.  More on this later.
>
>        try :
>            fIncl = open(mensaje.direcciones)
>        except Exception, e :
>            logging.error('Error!!! No pude abrir "%s" : %s',
>                            mensaje.direcciones,
>                            e.strerror)
>            raise

This is one function.  It has the argument filename.
>
>        try :
>            fExcl = open(mensaje.excluidas)
>        except Exception, e :
>            logging.error('No pude abrir "%s" : %s',
>                            mensaje.excluidas,
>                            e.strerror)
>            fIncl.close()
>            raise

This is the same function with a different argument.

>
>    except : pass
>    else :

The outer exception block here does nothing.   If an exception
is raised then the following code won't be executed anyway.

>        mails = enumerate(
>                        set(addr.strip() for addr in fIncl)
>                        - set(excl.strip() for excl in fExcl))
>        fIncl.close()
>        fExcl.close()

This is a method that gets the addresses from the files specified in
mensaje.  It opens the files.  It reads the contents.  It closes the  
files.
It should probably go in mensaje.   You should use try-finally blocks
to close the open files.  (or with blocks in python 2.5)

>       miCorreo = Correo(mensaje)
>        miCorreo.connect()
>        empiezo = time.clock()
>        for nro, addr in mails :
>            if nro%mensaje.cuantos == 0 and nro > 0 :
>                miCorreo.disconnect()
>                time.sleep(mensaje.intervalo - (time.clock() -  
> empiezo))
>                if not miCorreo.connect() :
>                    logging.info('Terminando')
>                    return
>                empiezo = time.clock()
>            miCorreo.enviar(addr)
>        miCorreo.disconnect()

This is the real meat.  Everything before it can be reduced to one line.

-jeff


More information about the Tutor mailing list