There's got to be a better way to do code reviews for embedded systems projects.
By Jason Cohen, Smart Bear Inc.
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.