[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