[Baypiggies] code review or tool suggestion

Drew Perttula drewp at bigasterisk.com
Wed Nov 25 08:56:08 CET 2009


Simeon Franklin wrote:
> More to the point of Baypiggies though - if anyone wants to take a
> look at my code I'd be happy for any pointers.


     28	    usage = "usage: %prog [-ovt] file"

The second 'usage' is apparently ignored (in py 2.6.2)-- you can leave 
it out and the output looks the same (!).


     30	    parser.add_option("-o", action="store_true", dest="stdout", 
default=False,
     31	                      help="Print to stdout instead of modifying 
file")

Instead of dest, I like add_option("-o", "--stdout", action=...). 
Optparse chooses the long form for the option name, but the end user can 
now make more readable commandlines if he or she wants to.


     46	            for i, sub_part in enumerate(part.get_payload()):
     47	                replacement = process_node(sub_part)
     48	                if replacement:
     49	                    del(parts_list[i])

I don't have a proof, but this del seems like a bug. As soon as you 
delete an element out of the middle of parts_list, the rest of the 
parts_list wouldn't be in sync with the i index, so a second delete 
would hit the wrong part, no? This can surely be rewritten without any 
indexing.

Also, I don't think you need to repeat the get_payload call in 43 and 
46. Maybe you had a previous version that appended the new part right 
onto parts_list in line 50? (Although even if you did that, you still 
wouldn't need to repeat the get_payload call.)

48 makes me nervous because it's a sloppy check for a value that could 
be False or "" or "some text". I'd probably use None as the special 
token and "if replacement is not None:" on line 48.


     38	    def process_node(part):

This function is nested just so it can access 'options' from the outer 
function, as far as I can tell. It would probably be easier to read if 
process_node was at the top-level after main(), and you passed in 
'verbose' as an argument. That way, process_node wouldn't be 
unnecessarily coupled with main(), main would be shorter, other programs 
could import and use process_node.


     64	        print "No filename was specified"
     65	        exit(-1)
     66	    else:

raise SystemExit("No filename was specified")
would be similar, but it prints to stderr (good!) and exits with a 1 
(close enough!).

I think it would be quicker for readers to parse this as an error check 
if you dropped the 'else' and unindented 67-69.



More information about the Baypiggies mailing list