[Baypiggies] code review or tool suggestion

[Brent Tubbs]

I didn't know about "raise SystemExit" either.  I like it!  And I like the
idea of doing more code reviews in the group.

I wish we had a better tool for it than just email though, like the one
built into Google Code that lets you attach comments to lines of code.  I'm
not very good at understanding the context when I read lines pasted into an
email.

Brent

On Wed, Nov 25, 2009 at 7:44 AM, Glen Jarvis <glen at glenjarvis.com> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/baypiggies/attachments/20091125/0479ee22/attachment.htm>