| |||||||||||||
Otte: staring into the abyssOtte: staring into the abyssPosted Aug 4, 2012 12:37 UTC (Sat) by nix (subscriber, #2304)In reply to: Otte: staring into the abyss by jzbiciak Parent article: Otte: staring into the abyss Yep. And code like that is great if the data you are entering is really columnar. It's awful for e.g. the very assignments you show it used for, becaue it's actually quite hard for the eye to track from, say, one of those = 0's back to the thing you're assigning to -- you have to track across a lot of whitespace without deviating horizontally, which the eye is notably bad at. In this specific case it doesn't matter because the assignments are all either of 0, or at the end of a block, or of a name similar to the name of the variable, or there happens not to be much whitespace, but can you see how bad this close variant is? (reordering things to provide an example with enough horizontal whitespace to make my point, I know this makes no sense as code anymore):
stic->phase = 0;
stic->next_phase = 57;
stic->fifo_ptr = 0;
stic->req_bus->intrq_until = 0;
stic->req_bus->intrq_until = 0;
stic->req_bus->intrq = 0;
stic->stic_accessible = 14;
stic->gmem_accessible = 0;
It's actually quite hard to see which variable that 57 or 14 are assigned to: they're lost in a sea of horizontal whitespace, while the columnar format makes it appear that all those 0s and 14s and 57s are related to each other, when they are not at all. Isn't this clearer?
stic->phase = 0;
stic->next_phase = 57;
stic->fifo_ptr = 0;
stic->req_bus->intrq_until = 0;
stic->req_bus->intrq_until = 0;
stic->req_bus->intrq = 0;
stic->stic_accessible = 14;
stic->gmem_accessible = 0;
Another problem with lining things up like you suggest is that it encourages people to use tabs to line things up, rather than restricting themselves to using tabs solely in the left margin as sole indentation mechanism. And I don't need to tell you why that is an awful idea unless you have a way of enforcing that absolutely everyone on the project uses the same tab size. (I prefer to do as GNU is increasingly doing and simply ban tabs outside of literal strings. It's too easy for a thoughtless co-worker with the wrong tab size to turn a codebase into mush with a single save when tabs are in wide use. With spaces, this is impossible, even if you do lose the easy 'change the indentation by changing the tab size' trick. This trick rarely works in practice because most people don't have the discipline to use tabs and only tabs as indentation mechanism, with no padding out with spaces, and to use them nowhere else, ever.)
(Log in to post comments)
Otte: staring into the abyss Posted Aug 4, 2012 16:45 UTC (Sat) by jzbiciak (✭ supporter ✭, #5246) [Link] Actually, I find your compressed whitespace version harder to follow, probably because I'm in the habit of moving my eyes between columns and working fields in groups. I'll read a few field names (as in 2 to 4), then jump to the column of values, then back to the column of field names, etc. That is to say, my visual scanning is roughly like this:
stic->phase
stic->next_phase
stic->fifo_ptr
...followed by...
= 0;
= 57;
= 0;
All three values are centered together in the highest-res portion of my vision. That doesn't work as well when the second target is more like:
= 0;
XXXXX = 57;
XXX = 0;
If it makes sense to do so, I'll even insert a blank line to make the grouping more obvious, or use different horizontal columns for the values in the groups, if the groups are more logically separate. If I always scanned field name by field name one at a time, it'd be a problem. But since I tend to bunch fields in logical groups, and tend to treat them as groups, it's actually easier for me to follow when I can see all the field names simultaneously, and then see all the field values simultaneously. My brain can remember field order for a handful of fields at a time, and so treating them in groups goes much faster for me. Now, in C++, where all those fields would be constructor arguments instead, I'd probably group them differently. In any case, I try to group things to that my innate visual sense of "alike" and "different" help me spot anomalies. In your compressed version, I have to play "find the value", and I forget what value I'm looking for after awhile. I'm forced to read field-by-field, which is much slower for me. I'm /less/ likely to find unintended discrepancies. I guess it's the same reason I write "max" with this idiom:
if (max < val)
max = val;
It's easier to see I've hooked everything up properly if I can read down the columns and everything reads in the same order in each column. BTW, on the tabs thing: I've got a strict no-tabs policy in our team, and we set indentations at 4. It's easily enforced so far, though, because there's no more than a few of us at any given time. We're attached to a chip design team, so there's no danger of us suddenly growing our software team...
Otte: staring into the abyss Posted Aug 5, 2012 7:27 UTC (Sun) by dirtyepic (subscriber, #30178) [Link]
Conversations like this are why I love this site.
Otte: staring into the abyss Posted Aug 6, 2012 8:30 UTC (Mon) by nix (subscriber, #2304) [Link]
Aaah, thanks for that description. I think you've elucidated the key difference here, perhaps without knowing you did. Probably because you're attached to a chip design team, which tends to emit lots of stuff with big initialization blocks filled with heaps of magic constants, you're in the habit of *using* huge initialization blocks filled with magic constants and so have come up with a reading mechanism which ties those constants together, because those constants often really *are* related.
I've never worked on anything like that (I started out on financial database stuff, then as soon as I started working for a major database vendor I stopped working on databases and started working on stuff closer to the toolchain), and would consider heaps of simple assignments bunched together like that tantamount to lack of sufficient ingenuity: it's usually unnecessary code, since it's normally right next to the declaration anyway. So as long as it's something C would permit in an initializer, I'd probably use one (a designated initializer if necessary). Inside designated initializers, which are certain to contain a group of closely related initializations, I often *do* line things up as you suggest, as long as the amount of introduced whitespace is low -- so I think perhaps there is less difference between our coding styles than was at first visible.
My rule is simple: avoid the boring and evil (one is hard to write: the other is hard to read). What is boring to me is that repeated-over-and-over 'stic->' in your example: what is evil is a spaced-out initializer with a lot of space in which the eye gets lost. (Your solution there would probably be to group them differently so that variables with names of similar lengths were grouped together: mine would generally be to not space them out. I can't really call this a huge difference in style.)
In any case, there is one situation where spacing-out is always called for: same-line comments. *Nobody*, not even weird people who voluntarily use Hungarian notation, puts all their same-line comments right next to the code it describes unless the line is long. And not even in these days of 200-column xterms does anyone write a 200-column same-line comment.
My apologies for not turning this into a dark-curse-ridden my-way-or-/dev/null flamefest. I'm aware that I am below quota on these for this year and will lose my Internet access privileges unless I get some better flame up.
Otte: staring into the abyss Posted Aug 6, 2012 13:02 UTC (Mon) by jzbiciak (✭ supporter ✭, #5246) [Link] My rule is simple: avoid the boring and evil (one is hard to write: the other is hard to read). What is boring to me is that repeated-over-and-over 'stic->' in your example: what is evil is a spaced-out initializer with a lot of space in which the eye gets lost. In Pascal, I could have used a "with" block, but C lacks such things. In perl, I use the fat comma in a nicely indented initializer block:
my %hash =
(
foo => 0,
bar => 42,
baz => 1,
quux => 1234,
gronk => 0
);
The particular code I shared earlier (the STIC code) actually largely pre-dates wide availability of C's designated initializers. With a modern C compiler I would instead consider putting initializers like that block into const structures and use structure assignment to copy the block in in one go. That would both eliminate the boring repeated prefix, and would also likely reduce the overall code footprint since it would replace the discrete scalar assignments with a loop.
Otte: staring into the abyss Posted Aug 6, 2012 13:58 UTC (Mon) by nix (subscriber, #2304) [Link]
I believe we are in violent agreement then, modulo a tiny niggle over code style that should have no effect on modern code :)
|
|||||||||||||