Explaining variables
I recently came across an excellent book: The Art of Readable Code by Dustin Boswell and Trevor Foucher. As soon as I heard about the book, I knew that it would interest me and ordered a copy without delay. For years, I have pushed the message that the #1 priority, when writing code, is readability; the authors and I are on the same wavelength. I am likely to be referring to this book again in this blog, as, on initial reading, although many things are already clear and familiar to me, I still have more to learn and to share …
A common argument for writing complex, hard-to-read code is “efficiency”. Although this is an important matter for many embedded designs, where resources are limited, modern development tools really help to avoid unnecessary obfuscation. Let’s look at an example:
Imagine their is a device, with a control register located at address 0x80000004 and bit 2 indicates that the device has an error. In our software, we reach a point where we wish to execute some code if the device is showing an error and we might do that thus:
if ((*((unsigned *)0x80000004)) & 0x4) != 0) ...
This is far from readable and can be improved in obvious ways:
#define MYDEVICE (*((unsigned *)0x80000004)) if ((MYDEVICE & 0x4) != 0) // test error bit ...
A further improvement would be to use an intermediate variable to hold the result of the test – an “explaining variable”:
#define MYDEVICE (*((unsigned *)0x80000004)) unsigned DeviceError; DeviceError = MYDEVICE & 0x4; // test error bit if (DeviceError != 0) ...
Is that clearer? I believe that it is. However, I can hear a “non-believer” complaining: “That intermediate variable is an unnecessary overhead!” This would be true, except that a modern, optimizing compiler would certainly eliminate the variable if it were used just once. If It were to be used again, the variable would probably be instantiated, but the test would only be performed once, so the overhead is well worthwhile. In any case, it may be allocated to a machine register, which minimizes the impact.
Comments
Leave a Reply
You must be logged in to post a comment.
This is generally good advice. However, it can be improved with better types:
#define MYDEVICE (*((volatile uint32_t *) 0x80000004))
bool DeviceError = MYDEVICE & 0x4; // test error bit
if (DeviceError) …
It is also debatable whether it is a good idea to make #define’s for such register access. Structs are often better, and minimise the namespace pollution of macros. Another option is:
static inline uint32_t mydevice_read_status(void) {
return *((volatile uint32_t *) 0x80000004);
}
static inline bool mydevice_isError(void) {
return mydevice_read_status() & 0x04;
}
Names are good, but macros are not necessarily the best way to get them.
Let me try that again, and see if some blank lines make the post legible.
This is generally good advice. However, it can be improved with better types:
#define MYDEVICE (*((volatile uint32_t *) 0x80000003))
bool DeviceError = MYDEVICE & 0x04; // Test error bit
if (DeviceError) …
It is also debatable whether it is a good idea to make #define’s for such register access. Structs are often better, and minimise the namespace pollution of macros. Another option is:
static inline int32_t mydevice_read_status(void) {
return *((volatile uint32_t *) 0x8000004);
}
static inline bool mydevice_isError(void) {
return mydevice_read_status() & 0x04;
}
Names are good, but macros are not necessarily the best way to get them.
No arguments with any of the @David.
Colin, is there a way to make comments here that allow sensible formatting? I would like to comment on a number of your blog articles, but it would be a waste of time if the result is as illegible as the posts I have made here.
Very good read. One can make the code slightly more readable by defining bit #2 like #define CR_DEVICEERRORBIT 0x04 thus using CR_DEVICEERRORBIT instead of 0x04.
You certainly can add definitions for bit numbers like that. But be wary of doing that too much. If the bit number is only used in one case, are you really adding anything by having a macro definition? A macro definition with a name does not give the reader (or the compiler) any more information than using the bit number directly along with a comment – and it avoids adding more macros to the global namespace. I would also recommend adding a reference to the appropriate datasheet or reference manual chapter to the comments – it makes it far easier to see find out what the particular bit actually does.