10 Questions for a Successful Code Review

Over the years I’ve noticed a number of common gotchas when reviewing code. They’re there no matter what the size the company or how mature the development process (and I have had the opportunity to review software for companies ranging from those with strict and bureaucratic processes to those that are more shoot-first-aim-later). In order to help alleviate these common issues, there are 10 questions that can be asked while reviewing C code to help find potential bug-ridden areas.

Question #1 – Does the program build without warnings?

Code cannot be loaded on a target without a successful compile. A successful compile involves the programmer diligently removing any syntax errors in order to make the compiler happy and have it create an output file. But a compiler can build an application without error yet still find other anomalies, such as an implicit cast, that it reports as a warning. A truly successful compilation of a program, then, should involve not just compiling with zero errors but also with zero warnings. This definition of successful compilation may seem obvious, but failure to resolve warnings is an error that I continue to see from both green and senior engineers alike. The worst case I have seen had more than 100 warnings! The most disturbing case had just one: a simple implicit cast warning of an unsigned integer into a float.

Question #2 – Are there any blocking functions?

One of the primary purposes of a microcontroller (MCU), in my opinion, is to be able to handle real-time events. MCUs should be able to handle those events in a very deterministic manner that can be measured and proven. Yet one of the common mistakes that I often see is that a driver or some application code segment is written such that it enters a loop or calls a delay function for an extended period of time. But a loop or delay prevents any other code from running on the processor, potentially compromising determinism. Button debounce functions are a notorious example. Instead of polling the GPIO pin or setting up an interrupt, many debounce implementations read the pin, then enter a delay of 20, 30, maybe even 40 milliseconds before reading the pin again. Take a look at Figure 1 for an example. In an environment without an RTOS this delay is the death of a real-time system.

2016-09-figure1

Question #3 – Are there any potential infinite loops?

Who in their right mind would put an infinite loop into their code on purpose? (Excluding of course those needed in tasks or the application’s main loop.) Yet there is a lot of example code on the web and from silicon vendors that exhibits an infinite loop failure behavior. For example, code that writes data to flash or EEPROM typically will monitor a hardware flag for completion. Example code will create a while loop on the flag to reach a certain state before proceeding. But if the hardware fails and the flag never gets set, the code becomes stuck in an infinite loop! An example of this can be seen in Figure 2.

2016-09-figure2

One way that this infinite loop failure can be remedied is to have the loop monitor a system tick or to limit the number of times that the loop can execute before finally deciding that an error has occurred. These remedies allow for error handling to be built into the system in the event that the hardware fails. Despite popular belief, hardware (and software) does fail.

Question #4 – Should this function parameter be const?

Programmers tend to not use const as much as they should especially when it comes to function parameters. Declaring a passed function parameter as const is a great way to protect that variable from being accidentally modified within the function. Why leave it to chance that a future developer will realize that they shouldn’t be modifying that system-critical variable but should only be using it?

Question #5 – Is the code’s cyclomatic complexity less than 10?

Monitoring a function’s cyclomatic complexity metric is a great way to help limit how complex a function gets. This metric directly relates to the minimum number of test cases that need to be performed on a function to test every branch. Not only that, the metric really tells how much a developer needs to keep track of in their mind while they are writing or modifying a function. Since most humans can only keep track of seven to nine things at once, keeping cyclomatic complexity less than 10 is a good bet to help keep bug rates down.

Question #6 – Has extern been limited with a liberal use of static?

The C language defaults a variable’s scope to extern. This default is implicit; a variable declared in a module without the use of static has a big invisible extern sitting in front of it. The only way to get rid of that invisible extern is to place a visible static in front of the declaration. This practice has the added benefit of making the variable local in scope, aiding in data hiding and encapsulation. The most common place to look for implicit externs is module level variable declarations.

Question #7 – Do all if … else if … conditionals end with an else?

The use of a default case in a switch statement should be mandatory. Static analysis tools will complain if a default case isn’t present. A developer can easily see that if a conditional warrants the use of a switch statement with various cases, there may be a case that is unexpected or overlooked that should have a default end-all case. This applies to if … else if … conditionals as well. If two or more conditions are to be checked for, what should be done if none of these cases meets the current condition? The final else in the statement acts just like default case in a switch statement.

Question #8 – Are assertions and/or input/output checks present?

One of the very early lessons that I remember from programming classes in high school was that inputs and outputs to functions should be checked to ensure that they are valid. Unfortunately, despite this teaching embedded software developers tend to ignore this computer science wisdom. Embedded software developers should be sprinkling their code with assertions to validate that their assumptions about the program’s behavior at certain points is correct. Boundary checks should be performed on inbound and outbound data. Remember the old saying “garbage in, garbage out?”

Question #9 – Are header guards present?

A header guard is a simple macro that ensures that the header file is not included more than one time within a translational unit. The guard prevents double inclusion of the #include directives. Not including the header guards can result in some very strange static analysis behavior. More importantly, using a guard prevents multiple definition errors.

Question #10 – Is floating point mathematics being used?

The use of floating point mathematics can be a sticky subject in embedded systems. Resource-constrained microcontrollers often do not include a floating point unit (FPU). This absence means that the processor has only one means of performing floating point calculations: using a library function. Library functions for floating point math are usually slow and inefficient, they don’t necessarily have deterministic behavior, and they can cause code size to balloon. For these reasons developers should carefully consider when to use floating point within a microcontroller. They should also perform additional testing and should consider alternative methods such as look up tables, scaling, and fixed point math.

Conclusion

Many engineers find code review to be extremely boring, but it is actually quite interesting. It is one of the aspects of consulting that I find to be quite enjoyable because performing a code review can be a very exciting time. Each programmer has their own unique view of and insights into embedded software development and of the C language that we have all come to love and hate, so there is always something to learn. Yet despite the many insights and varying levels of checks and balancing that developers are implementing, errors persist. These ten questions address common errors and misconceptions with developing embedded software that should be checked with every code review.

Leave a Reply

Your email address will not be published. Required fields are marked *