Friday, December 2, 2011

Improving code readability through temporary variables

We don't always have the luxury of using library interfaces that are sensibly designed and enforce readability self-explanatory (see this presentation). A fictional function may look like this:

extern void foo(struct foo *f,
Bool check_device,
int max_devices,
Bool emulate);

But a calls often end up like this:

foo(mystruct, TRUE, 0, FALSE);

Or, even worse, the function call could be:

foo(mystruct, !dev->invalid, 0, list_is_first_entry(dev->list));

The above is virtually unreadable and to understand it one needs to look at the function definition and the caller at the same time. The simple use of a temporary variable can do wonders here:

Bool check_device = !dev->invalid;
Bool emulate_pointer = list_is_first_entry(dev->list));

foo(mystruct, check_device, 0, emulate_pointer);

It adds a few lines of code (though I suspect the compiler will mostly reduce the output anyway) but it improves readability. Especially in the cases where the source field name is vastly different to the implied effect. In the second example, it's immediately obvious that pointer emulation should happen for the first entry.


Anonymous said...

So, you're trying to add extra information to the code in order to make it more understandable to the humans who are reading it. Why not just ... use comments?

!dev->invalid, /* check_device */
0, /* max_devices */
list_is_first_entry(dev->list)); /* emulate_pointer */

If I saw your second version of the code, I'd assume that it was a left-over wart from some previous code, where, e.g. calculating the value of the check_device flag was a lot more complex, perhaps conditional on something, and the clean-up didn't quite go as far as it could. Then I'd submit a patch to finish tidying things up.

If you want to document the code for humans, use a comment. That's what they're there for. And they're meta-self-documenting about the fact that they are documentation.

Yes, comments can be overused. But just because the apocryphal /* increment i */ is as pointless as the flamboyant over-use of "goto" is nightmarish, that is no reason to completely ban either comments or goto. Both have their place, and should be used where appropriate.

Luke Hutchison said...

I always just do:

foo(mystruct, /* check_device = */ !dev->invalid, /* max_devices = */ 0, /* emulate = */ list_is_first_entry(dev->list));

Anonymous said...

Eh, I'm going to have to second the author on this one. Well written code should be self-documenting. Let the compiler do its job. Temporary variables of this sort are nearly always optimized out (I've never seen an instance where they weren't). If you need a comment (or multiple comments) to explain a single line of code, you should probably rewrite it.

David said...

Named parameters is a functionality I miss in C...

In Ada, you could write:

foo (f => mystruct,
check_device => !dev->invalid,
max_devices => 0,
emulate => list_is_first_entry(dev->list));

And in case of API change, the compiler would bug you.

Rafi said...

Not just Ada. Python, R, and Perl (sort of), off the top of my head, all support named parameters.

In C, I've seen people use empty #defines to annotate parameters.