[Baypiggies] code review or tool suggestion

Simeon Franklin simeonf at gmail.com
Wed Nov 25 22:50:34 CET 2009


Whoops - meant to send this to the list...

---------- Forwarded message ----------
From: Simeon Franklin <simeonf at gmail.com>
Date: Wed, Nov 25, 2009 at 12:06 PM
Subject: Re: [Baypiggies] code review or tool suggestion
To: Drew Perttula <drewp at bigasterisk.com>


Wow Drew - thank you so much. I definitely owe you a post-baypiggies
drink sometime. I've updated the code at github
(http://gist.github.com/242055) with my changes taking into account
your comments below.

I followed your suggestions on optparse style - using long options
instead 'dest' makes sense and gives flexibility to the command line.
The real meat was in the handling of multipart sections - you found a
bug plus just noticed the general untidyness left from ad-hoc
refactoring.

I didn't rewrite it without indexing - the index captures the order of
the attachments and is significant - but instead I kept a dict with an
index:replacement mapping and used the dict to update the list with
the original parts of the container. This also made the code (I think)
a bit more intelligible; what was

42        if part.is_multipart():
43            parts_list = part.get_payload()
44            new_parts_list = []
45            modify = False
46            for i, sub_part in enumerate(part.get_payload()):
47                replacement = process_node(sub_part)
48                if replacement:
49                    del(parts_list[i])
50                    new_parts_list.append(replacement)
51                    modify = True
52            if modify:
53                part.set_payload(parts_list + new_parts_list)

is now

51 if part.is_multipart():
52         parts_list = part.get_payload()
53         new_parts = {}
54         for i, sub_part in enumerate(parts_list):
55             replacement = process_node(sub_part, options)
56             if replacement is not None:
57                 new_parts[i] = replacement
58         if update_list_from_dict(parts_list, new_parts):
59             part.set_payload(parts_list)

I also incorporated your suggestion about the return value of
process_node - it made sense to me to "sloppy" check the return value
since it will either be an object (True) or an explicit False. More
explicit however is to return None (which semantically makes sense -
the function returns a replacement or None) and then the check reads
like english: if replacement is not None...

I still don't feel thrilled with my understanding of the email/MIME
API but I do feel better about my code... Thanks for taking the time
to take a look! I'd be happy to return the favor at any time.

-regards
Simeon Franklin


More information about the Baypiggies mailing list