Build checklists for embedded software projects
There's got to be a better way to do code reviews for embedded systems projects.
By Jason Cohen, Smart Bear Inc.
Embedded.com
(07/09/08, 03:40:00 PM EDT)
Code review checklists are usually a pain. They're often ridiculous in length or content. They're not fun to use. Sure we want processes to find defects in our code, but there's got to be a better way than a "Checklist for C" the boss downloaded from some website. Checklists can be an excellent way of finding defects early in the development process, but most of the time checklists are so impractical that they're more of a hindrance than a help.

Twenty-eight--that's the average number of checklist items I found in the top seven sample checklists from the Internet (according to Google, the Knower Of All). My field experience is similar; it's not unusual to find two pages of items, each requiring due consideration before its own white square can be checked.

Mammoth checklists are built with the idea of completeness. The list is usually divided into categories: correctness, maintainability, style, error handling, performance, mind-numbing, and so on. Items range in scope from generic concepts like "Function does what it is documented to do" to specific checks like "strncpy() is used instead of strcpy()."

In reality, these checklists are useless. If someone stacks 20 files on your desk and insists that a 30-item checklist be applied to each file, how would you even go about that? On the 12th file are you really going to be diligent on every item?

Here's what actually happens. People use the list as a general guide. They don't consider every item in every file. Which items will they remember as they look at the code? Will it be the most important ones?

Magic numbers
In 1956, George Miller published a now-famous paper called "The Magic Number Seven, Plus or Minus Two." He measured human short-term memory capacity for "chunks of information." For example, most people can remember an American seven-digit local number long enough to dial it, whereas when dialing a ten-digit long-distance number, most people have to refer back to the number at least twice.

Miller's results have been applied to various aspects of psychology and even computer science. For example, Miller's work is frequently cited as a reason for limiting a function's cyclomatic complexity to nine.

If we expect reviewers to think about the entire checklist when reviewing a file, we have to keep the checklist short. Five items sounds good; maybe as many as ten. So that's the first rule: List no more than ten items.

Getting away with five
With only a few checklist items, how will we still catch the same number of errors with code correctness, maintainability, and style? In my experience, long checklists contain mostly simple programming rules or coding style checks. Style items include naming conventions, whitespace, block positions, and comments usage. For embedded C programs, style rules are things like "Use underscores to separate words, not CamlCase" and "All functions must have a comment block in a certain format."

Simple programming rules identify programming errors and are often specific to a language. Some common C rules are "Always check function return values for error conditions" and "When filling a buffer provided by the caller, always require the caller to specify the buffer size and return error rather than overflowing."

All these rules can be enforced by static analysis tools. With both C and Java, the two most popular embedded software languages, there are dozens of tools, both free and commercial, that check for style and basic programming errors. Some cost money to buy and all cost time to set up, but it's not more expensive than having a human spend time digging up these mundane errors during a code review! Developers need to be doing things that only a human can do, like checking code correctness, maintainability, and sensible documentation.

The second rule is: List no items that can be automated. An item that appears on almost every checklist is: "Does the code accomplish what it is meant to do?" Is this question really necessary? It's a code review--what else would you expect the reviewer to do? Another common one: "Can you understand the code as written." Duh!

Checklist items are precious; don't waste them. With each one, ask yourself: Do I really need to remind my reviewers of this? The third rule is: List no obvious items.

What's left?
Before we had a proper build system, we manually updated the build number in a header file. Of course, we'd frequently forget to kick the number and, even with code review, the reviewers forgot to check for that too. After all, if that header file wasn't included in the review, there was nothing to jog the reviewer's memory. It's one thing to verify something that's put in front of you; it's a lot harder to review that which is missing.

If it's easy for the author to forget, the reviewer will forget too. This is where a checklist is the right tool. So the fourth rule is: List items that are commonly forgotten.

Here's an example of a perfect checklist item: "Recovers properly from errors everywhere in the function." Incorrect error-handling often results in fatal crashes or memory leaks; two things that are unacceptable for most embedded device applications. In my experience, few reviewers remember to look at error paths in the middle of a function, and authors forget too, especially when a change is made in a large function. You can't generally automate the check for correct unwinding, especially when the function is changing shared state like global variables or accessing external resources like an SPI bus.

How to build the list
The question I get asked most about checklists is: "Do you have a sample checklist we can use?" As a politician might say, you're asking the wrong question. The right question is: "How do we build a checklist appropriate for our software development group, and how do we adjust the list over time?"

Here's a weird experiment. For one week, every time you make a mistake, no matter how trivial, write it down. Everything! Misspell a word while writing an e-mail? Write it down. Close an application when you meant to close just one window? Write it down. Have a lingering compiler error when you run gcc? Write it down. Do this for a whole week.

This is infuriating. I don't know anyone who's actually made it through an entire week of this torture. You make lots of mistakes, and they're tiresome to chronicle. But you'll notice something interesting: you make the same kind of mistakes over and over. In my case, for example, I always misspelled the word "definitely," and I apparently use it all the time.

Of course, the next step is to correct the problems. Many of them correct themselves just by virtue of bringing them into conscious thought. Others errors you can correct with automation. For example, I use the auto-correct feature in Outlook to correct many misspellings.

Software checklists should be built exactly the same way. Look at the last one or two hundred defects found by code review and you'll find patterns. This isn't as hard as it sounds; you don't need detailed analysis or five columns of data on each defect. Just summarize the defects and the patterns will jump out.

A checklist derived from these patterns will be tuned specifically to the errors you make most often. So the fifth rule is: Let empirical evidence determine the checklist.

After you build it, and after they come, you still need to maintain it
This technique provides the initial checklist, but it turns out that the patterns change over time. Just as with the personal introspection project, everyone will get used to finding and fixing the same problems. Soon, code authors will think about these same things while the code is being written. The problem will be fixed before it starts.

At this point, the number of defects associated with that checklist item will diminish, eventually becoming so infrequent as to make that item useless. This is a good thing--developers have developed habits and techniques to avoid this type of error. Now you can replace that item with the next most common pattern.

The sixth rule: Track defects by item so you can replace stale items.

As if a checklist for checklists isn't recursive enough already, I'd like to note that this short list contains items (apparently) not obvious to most checklist designers and was built empirically from patterns I've found in the field.

Jason Cohen is the founder of Smart Bear Inc., makers of tools that assist developers and managers with lightweight peer code review. He's also the author of a popular book on this topic. Cohen can be reached at jason.cohen@smartbearsoftware.com.