In message <878w12kt5x.fsf....@metalzone.distorted.org.uk>, Mark Wooding wrote:
> Lawrence D'Oliveiro <l...@geek-central.gen.new_zealand> writes: > >> Maybe you should look at the code in context >> <https://github.com/ldo/dvd_menu_animator>, then you can express some >> more opinions on how to improve it. > > 1. class keyval: `are there official symbolic names for these > anywhere?' The gtk.keysyms module contains `Return', `KP_Enter', > `Left', `Up', `Right', and `Down'. Thanks, that’s useful. I couldn’t find mention of such in the documentation anywhere. > Question: why are `returnkey' and `enterkey' defined in hex and the > others in decimal? Can’t remember now. Does it matter? > 2. The MainWindow class only has the `Window' attribute described in > its definition. Apparently there are other attributes as well (the > ColorMumbleList family for one, also `LayerDisplay'). Similarly, > the LoadedImage class has attributes `Contents' and `FileName', but > I see attributes `RowStride', `ImageDrawSize' and `SrcDimensions' > used immediately below. These are really just namespaces, one for the GUI context and the other for the image data. > 3. In SelectSaveMenu, there's a curious `for' loop: > > for State in (gtk.STATE_NORMAL,) : # no need for > gtk.STATE_INSENSITIVE > ## blah > > Firstly, this is obviously equivalent to `state = gtk.STATE_NORMAL' > followed by the loop body (YAGNI). It’s a tuple of one element. It used to be a tuple of two, and there is the possibility it might need to become that again (as intimated at in the attached comment). That’s why it stays a tuple. > 4. Ahh! I've tracked down where the MainWindow is actually > populated. Ugh. The code appears to revel in boilerplate. (For > those at home, we're now looking at `SetupMainWindow', specifically > at the calls to `DefineScrollingList'.) I count six calls, each > nearly (but not quite) identical, of about eleven lines each. Could be worse. Imagine if I had written out the 30 lines of that function out each time instead. > Of course, being boilerplate, the similarities visually outweigh the > differences, when in fact it's the differences that are > interesting. That is generally how code reuse works. All the “boilerplate” is in the function definition, so I just have to call it parameterized by the differences appropriate to each instance. > It doesn't help that all of the names of the things involved are so > wretchedly verbose. Oh please, no. Since when are explanatory names “wretchedly verbose”? > How about the following? > > def make_listview_l1 ... And what, pray tell, is the significance of the name “make_listview_l1”? If there is something I would describe as “wretched”, it is the use of random numerical suffixes like this. > Looking at some of the rest of the code, it might (or might not) be > worthwhile actually making a class to gather together a list's model > and view; subclasses can then vary their individual parameters, and > the various colour lists can also keep the various captions with > them. Most of that variation is already handled without the limitations of thinking in classes. For example, selection of a colour in all three lists is handled through a single EditColor routine. And of course you’ve already seen how a single DefineScrollingList routine can be used to set up all the scrolling lists used in this GUI. > 5. Much of the indentation and layout is rather unconventional, though > not intolerable. But I found (deep in `SelectSaveMenu'): > > NewRenderPixels = array.array \ > ( > "B", > FillColor > * > (ImageRowStride // 4 * ImageBounds[1]) > ) > > to be most disconcerting. Is the structure of the expression not apparent to you? You’d probably need plenty of fresh air after something like this, then: PatternArray = array.array \ ( "I", 16 * (16 * (Light,) + 16 * (Dark,)) + 16 * (16 * (Dark,) + 16 * (Light,)) ) > Also, I couldn't work out why some parens are indented only two > spaces and others are indented the full eight. Eight?? You mean four. See, this is why I should probably stop using tabs... -- http://mail.python.org/mailman/listinfo/python-list
