In article <iar48g$hn...@lust.ihug.co.nz>, Lawrence D'Oliveiro <l...@geek-central.gen.new_zealand> wrote:
> > I suppose readability is in the eye of the reader, but, yes, I agree > > that I have split this into two parts. The parts are > > > > 1) The table of data > > > > 2) The looping construct > > But the table of data makes no sense outside of the looping construct. That > table does nothing other than define the bounds of the loop. I look at it the other way around. The data is the important part, and the loop is just plumbing. Once I've got the data in an easy to manage form, I can do lots of things with it. I could iterate over it (as originally written) with a "for" loop. I could refactor the code to pass the data to some function which processes it. I could refactor it a different way and use map() to process the data. > Which means you donât understand the purpose of the code at all. Go look at > it in its entirety, and youâll see what I mean. > > <http://github.com/ldo/dvd_menu_animator> That URL takes me to a github page. Can you be more specific about which file I should be looking at? I did take a look at one file, overscan_margins.py. Here's a few stylistic comments. 1) I like the fact that the file starts out with a block comment describing what it does, how to install it, and how to run it. Documentation is A Good Thing, and most people don't do enough of it. 2) You have provided comments for each function, such as (lines 36-37): def NewLayer(svg, LayerName) : # adds a new layer to the SVG document and returns it. This is good, but would be even more useful if it were turned into a docstring, such as: def NewLayer(svg, LayerName) : "adds a new layer to the SVG document and returns it." For the same amount of typing, you now have a piece of text which can be retrieved by help() and processed in various useful ways with other tools. 3) I find the deeply nested style you use very difficult to read. For example, on lines 80-103. As I read this, here's how I mentally process it: "OK, here's a function call (minor stumble over the open paren not being on the same line as the function name, but I can get past that). The first argument is TheLayer. The second argument is whatever inkex.addNS() returns. Umm..." At that point, I can't scan quickly any more. It took me a while to understand that the third argument was a dictionary. The nesting is just too deep for me to continue to mentally hold the context of "I'm looking at a function call and gathering up the arguments" while I drill down through the dictionary structure to understand what it is. This would be a lot easier to scan if it were written as: inkex.etree.SubElement(TheLayer, inkex.addNS("path", "svg"), stuff) where "stuff" is the big complicated dictionary, which is defined before the call. Of course, "stuff" is not a useful name for it, but not being familiar with the domain, I don't know what a useful name would be. Hmmm, googling for inkex.etree.SubElement found me some Inkscape documentation. Looks like "attribs" would be a good name for the dictionary, since that's the name they use in the docs. In fact, if you look at http://tinyurl.com/24wa3q8, you'll see they use exactly the style I describe above (modulo whitespace).
-- http://mail.python.org/mailman/listinfo/python-list
