[Baypiggies] code review or tool suggestion

[Glen Jarvis]

I learn *so* much from code reviews....

Just reading this conversation has helped me tremendously.... For  
example, raising SystemExit is not something I've done before as a  
clean way to exit. I always used sys.exit (or for rare cases _exit ).  
Now I know better.

Obviously with reviews, one must take the reviewer into account as  
well as the comments that they make. (Everyone has an opinion.. Often  
one gets conflicting opinions on the same issue. And some reviewers  
even give advise that breaks PEP-8 with the preamble... "*I* like to  
do...." or give obviously unpythonic advise)

However, Drew's advise was very Pythonic and helpful. I *really* liked  
reading Drew's comments because it showed readability was important to  
him, that he has knowledge of the language (what works in what  
version), he has looked at potential bugs, and, of course, that  
exercising code/code coverage is important.

No matter how good we are, we can all learn from code reviews. I'm  
personally *very* interested in this theme if any one else is brave  
enough to stick their neck out.

I think I'll dig out something and publish to the list the next few  
days, asking for a review. I'll try to find something very small to  
not overwelm the readers. I'll also put on my thick skin and remember  
that any comments made can make me grow as a programmer.

Anyone else?


Glen


On Nov 24, 2009, at 11:56 PM, Drew Perttula <drewp at bigasterisk.com>  
wrote:

> 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.
>
> _______________________________________________
> Baypiggies mailing list
> Baypiggies at python.org
> To change your subscription options or unsubscribe:
> http://mail.python.org/mailman/listinfo/baypiggies