[Baypiggies] code review or tool suggestion

[jim]

+1111111 
great possibilities! 
terrific to share the heart of our focus.
could be good fodder for newbie nuggets.  


On Wed, 2009-11-25 at 06:44 -0800, Glen Jarvis wrote:
> 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
> _______________________________________________
> Baypiggies mailing list
> Baypiggies at python.org
> To change your subscription options or unsubscribe:
> http://mail.python.org/mailman/listinfo/baypiggies
>