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.[/QUOTE]
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).