L
Lawrence D'Oliveiro
Tufte’s concept of “chartjunk†could perhaps be extended to “formatjunk†or
“prettyprintjunkâ€.
Tufte’s concept of “chartjunk†could perhaps be extended to “formatjunk†or
“prettyprintjunkâ€.
S
Steven D'Aprano
Perhaps, but lining up the columns in a 3x3 table is hardly "ornate".
I have mixed feelings here. On the one hand, there's no doubt in my mind
that the extra whitespace and aligned columns make the data easier to
read for me. On the other hand, I know that it is easy to fall into the
trap of spending hours carefully aligning code that doesn't need to be
aligned.
R
Roy Smith
Lawrence D'Oliveiro said:
Not at all. Tufte is all about making data easy to understand visually.
The chartjunk he rails about is colors, shadows, 3-d effects, etc, which
make a chart "look pretty" but don't add to comprehension. If you take
out the extra whitespace, you end up with this:
which I think is more difficult to scan visually. In the first example,
I can see from the overall layout, without reading a single character of
punctuation, that this is a sequence of groups of three items. That's
harder to see in the second example.
Likewise, if I want to know what kinds of things are in the second field
of all the items, it's easy for me to visually scan down the second
column in the first example. To do that in the second example is much
more difficult.
If you want to talk about "codejunk", my biggest concern here is the
things in the third column. They all start with "MainWindow.Color", and
they all end with "List". Thus, those bits of text add bulk, and no
useful information. It would be even more readable by doing:
which, of course would require some more code elsewhere to define the
new names. Whether this makes sense in the context of the overall
codebase is an open question, but taking the table in isolation, it
certainly improves the readability. Tufte would approve.
L
Lawrence D'Oliveiro
Precisely my point.
Not surprising, since the above list has become completely divorced from its
original purpose. Anybody remember what that was? It was supposed to be used
in a loop, as follows:
for \
Description, Attr, ColorList \
in \
(
("normal", "image", MainWindow.ColorsNormalList),
("highlighted", "highlight", MainWindow.ColorsHighlightedList),
("selected", "select", MainWindow.ColorsSelectedList),
) \
:
...
#end for
Does this make more sense now?
S
Steven D'Aprano
No, it makes less sense, and is ugly too boot.
M
Mark Wooding
Lawrence D'Oliveiro said:
Ugh, no!
for descr, attr, colours in [
('normal', 'image', 'Normal'),
('highlighted', 'highlight', 'Highlighted'),
('selected', 'select', 'Selected')]:
colourlist = getattr(MainWindow, 'Colors%sList' % colours)
## ...
To be honest, there's so much regularity in that table that I'd consider
generating it from a shorter list. This would obviously involve
refactoring something else to eliminate the image/normal asymmetry. I'd
also consider making the ColorsMumbleList attribute collection into a
dictionary.
Note that my columns are aligned to 8-column tab stops for easy
maintenance (although my editor has used physical spaces rather than
tabs).
-- [mdw]
N
Neil Cerutti
Some sort of comprehension test is probably required to know for
sure, but I doubt the code benefits much at all from the columns.
styles = [
("normal", "image", MainWindow.ColorsNormalList),
("highlighted", "highlight", MainWindow.ColorsHighlightedList),
("selected", "select", MainWindow.ColorsSelectedList)]
Moreover, I jump the gun early on tables like this.
styles = [csv.reader(open("styles.csv", newlines=''))]
Maybe too early.
L
Lawrence D'Oliveiro
Mark Wooding said:
But then you lose the ability to match up the bracketing symbols. That’s why
I put them on lines by themselves.
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.
M
Mark Wooding
Lawrence D'Oliveiro said:
You're right. I do.
But the bracketing symbols are thoroughly uninteresting! All that
putting them on their own lines achieves is to draw attention to the
scaffolding at the expense of the building. It's great if you like the
Pompidou Centre, I suppose...
Besides, any editor worth its salt will highlight bracket mismatches for
the reader, or at least get its automatic indentation in a twist if you
get the brackets wrong.
1. class keyval: `are there official symbolic names for these
anywhere?' The gtk.keysyms module contains `Return', `KP_Enter',
`Left', `Up', `Right', and `Down'. Question: why are `returnkey'
and `enterkey' defined in hex and the others in decimal?
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.
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). Secondly, the loop is followed
by an `#end if' comment. If you're going to write the things, at
least get them right!
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. Of
course, being boilerplate, the similarities visually outweigh the
differences, when in fact it's the differences that are
interesting.
It doesn't help that all of the names of the things involved are so
wretchedly verbose. How about the following?
def make_listview(label, render, colattr, head, bounds, osc, box):
setattr(MainWindow, label + 'Display',
DefineScrollingList(getattr(MainWindow, label + 'List'),
0, render, colattr, head,
bounds, osc, box)
def make_listview_l1(label, head, osc):
make_listview(label, None, 'text, head, (160, 160), osc, List1Box)
make_listview_l1('Button', 'Buttons', None)
make_listview_l1('Layer', 'Button Layer', LayerSelectionChanged)
MainWindow.colourlist = {}
MainWindow.colourview = {}
for head in ['Normal', 'Highlight', 'Select']:
MainWindow.colourlist[head] = gtk.ListStore(gobject.TYPE_PYOBJECT)
MainWindow.colourview[head] = \
DefineScrollingList(MainWindow.colourlist[head],
0, ColorCellRenderer(), None,
head, (120, 120), None, List2Box)
make_listview('LoadedColors', ColorCellRenderer(), None, '',
(120, 240), None, MiddleBox)
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. (Sometimes a strong separation between model and view is a
good thing; this doesn't seem to be one of those times.)
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. Also, I couldn't work out why some
parens are indented only two spaces and others are indented the
full eight. Oh! It's inconsistent tab/space selection.
I'm afraid that about this point I had to get on with some other stuff.
I've done enough throwing of stones, so I should point at a glass house of
my own so that others can return the favour:
http://git.distorted.org.uk/~mdw/tripe/tree
The Python code is in `py', `svc' and `mon'. Those who believe PEP 8 is
carven in stone will be disappointed. It's a bit... tricky in places.
-- [mdw]
R
Robert Kern
For me, putting the brackets on their own lines (and using a trailing comma) has
little to do with increasing readability. It's for making editing easier.
Keeping all of the items consistent means that in order to add, delete, or move
any item is the same operation everywhere in the list whether it is the first
item, last item, or any item in between.
--
Robert Kern
"I have come to believe that the whole world is an enigma, a harmless enigma
that is made terrible by our own mad attempt to interpret it as though it had
an underlying truth."
-- Umberto Eco
T
Tim Chase
It also makes diff's much easier to read (my big impetus for
doing the same as Robert)
-tkc
M
Mark Wooding
Tim Chase said:
Hmm. That's a good point, actually. I'm not overly fussed about the
ease of editing: it doesn't seem especially hard either way. But nice
diffs are definitely valuable. Food for thought; thanks.
-- [mdw]
T
Tim Chase
I find that diffs like
for row in (
(item_bar, details_bar),
(item_foo, details_foo),
+ (item_baz, details_baz),
):
do_something(row)
are much easier/faster to understand instead of
for row in (
(item_bar, details_bar),
- (item_foo, details_foo)):
+ (item_foo, details_foo),
+ (item_baz, details_baz)):
do_something(row)
-tkc
R
Robert Kern
It might just be my vim bias. Vim has such *very* easy ways to cut and paste
whole lines that not taking the opportunity to permit them seems like a waste.
--
Robert Kern
"I have come to believe that the whole world is an enigma, a harmless enigma
that is made terrible by our own mad attempt to interpret it as though it had
an underlying truth."
-- Umberto Eco
L
Lawrence D'Oliveiro
Robert Kern said:
Yup, I like to do that when there’s nothing special about the last item in
the list. Sometimes there is (e.g. an enumeration entry naming the number of
values in the enumeration, or an all-zero sentinel marking the end of a
list), in which case I omit the comma to indicate that nothing should come
afterwards.
I remember an early experience with JavaScript (back in the days of Netscape
versus Internet Explorer 5.1 on a Mac), when I found that constructing a
list in this way wasn’t working in IE: turned out it was inserting some kind
of extra phantom list item after that last comma. Sigh...
L
Lawrence D'Oliveiro
Mark Wooding said:
Thanks, that’s useful. I couldn’t find mention of such in the documentation
anywhere.
Can’t remember now. Does it matter?
These are really just namespaces, one for the GUI context and the other for
the image data.
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.
Could be worse. Imagine if I had written out the 30 lines of that function
out each time instead.
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.
Oh please, no. Since when are explanatory names “wretchedly verbose�
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.
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.
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,))
)
Eight?? You mean four.
See, this is why I should probably stop using tabs...
M
Mark Wooding
Lawrence D'Oliveiro said:
I think my point was about the rather selective documentation of the
namespaces. The trivial-class trick is something I use myself (though
usually to simulate a structure rather than just to carve up namespace)
so I don't find it objectionable in and of itself.
I guessed the history. I'm in two minds about this sort of thing. I
/like/ history: I'm fascinated by the history of old programs and
programming languages; but looping over a single thing just seems weird.
Except that's not the case. There are extremely close similarities
between the six calls which strongly suggest (to me, anyway) that
further factoring would be beneficial. I detest writing anything more
than once, and don't much enjoy playing spot-the-difference when reading
code. The six calls are a screenful of spot-the-difference.
When the start and end bits are the same and the different bits are
hidden in the middle. After staring at a screenful of
MainWindow.MumbleDisplay my head starts spinning.
It's a terse reference to List1Box (your name). Since it was only being
used in the following two lines, I figured the name didn't matter much.
(In Lisp, I'd have wrapped the two lines in FLET and simply called the
function FROB.)
There's so much commonality in the arguments that I'm not convinced that
they're fully factored. Worse, there are several tables involving your
various ColorMumbleLists: adding another would require fiddling with all
of them, which suggests that things have been sliced up the wrong way.
(I'm aware in this instance that the set of such things is externally
constrained in this case.) Maybe packaging all of the information about
each individual list in one object and having a list of these objects
would be better.
No, it really isn't. Oddly enough, I'd find
(setf new-render-pixels
(array:array "B"
(* fill-color
(floor image-row-stride 4)
(aref image-bounds 1))))
much easier to cope with.
That's not noticeably worse. The bizarre thing is the relative
indentation of the operands to the * -- the oddness of which is
emphasized by the initial argument.
No, eight. Text editors are misleading: tab stops for plain text files
are not a matter of individual preference. (I read the code straight
off GitHub: my browser correctly interpreted tabs as moving to the next
multiple of eight columns.)
-- [mdw]
R
Robert Kern
It's one of the things that pains me every time I code JavaScript. That and the
lack of tuple unpacking.
--
Robert Kern
"I have come to believe that the whole world is an enigma, a harmless enigma
that is made terrible by our own mad attempt to interpret it as though it had
an underlying truth."
-- Umberto Eco