Dr. Dobb's is part of the Informa Tech Division of Informa PLC

This site is operated by a business or businesses owned by Informa PLC and all copyright resides with them. Informa PLC's registered office is 5 Howick Place, London SW1P 1WG. Registered in England and Wales. Number 8860726.


Channels ▼
RSS

Design

Code Inspection Book


I follow a number of companies that offer tools to embedded developers. We humans are tool builders; we engineers in particular create and use tools ranging from the physical (e.g., a wrench or logic analyzer) to the ethereal (software like compilers). Bereft of tools we'd be utterly unable to do our jobs. Just think of our helplessness when the power fails! Here we stare dumbly at each other for a minute or two before going out for ice cream or a drink if it's late afternoon.

Smart Bear Software  offers a number of tools to all sorts of software developers. Their $489 Code Collaborator is an intriguing program that supports lightweight code reviews. Lightweight, because the traditional code inspection advocated by Michael Fagan and Thomas Gilb doesn't fit the needs of some teams, but the benefits of inspections are so profound that even the smallest outfits must take advantage of the technique.

Now Smart Bear has a book on the subject which summarizes the results of 2500 reviews of 3.2 million lines of code at Cisco. Using Code Collaborator, which automatically generates and preserves metrics, they were able to learn a lot about lightweight reviews in a real-world scenario. Too many studies are set in academic communities using undergraduate students on toy projects.

The authors rightly point out that Fagan, Gilb, et al promote a highly formalized process that we're not supposed to tune much. Yet a Fagan inspection simply isn't possible in a two-person shop. And sometimes it's hard to match the process of the Fagan approach to modern agile methods. Indeed, in eXtreme Programming, for better or for worse the review takes place in real-time as a pair of developers crank code sharing a single machine.

The book refers to a number of studies, some of which are relatively obscure. For instance, did you know that when reading a function developers repeatedly return to look at variable definitions? The implication is that short term memory doesn't hold a lot, so wise teams will insist that all functions fit on a single page. Then it's easy to glance up at the declarations without shuffling through paper or screens.

The Cisco study showed a tremendous variability in inspection rates for a lot of reasons. But engineers achieved the best results when inspecting at about 300 lines of code per hour or less. And after about an hour review effectiveness plummets. We get tired.

Expect to find about 15 defects per hour.

Authors who "annotate" and explain the code before the review have fewer mistakes. It's not clear from the book what "annotates" means or how this is different than decent commenting. But clear explanations to someone else, presumably in written form, makes the author think more deeply and thus find his or her own problems before the review takes place.

It's a very well-written 164 page book that's a fascinating read. Even better, "Best Kept Secrets of Peer Code Review," is free! Go to online at SmartBear to order it. It's a physical volume, not a PDF which is a pain to read on-screen and annoying to print. Yes, it promotes their product, but the authors wisely relegated the sales hype to the last chapter. And do read that section carefully, too. Any tool that can help improve your processes and reduce defect rates is worth investigating.

What do you think? Have you read the book? Do you do code reviews of any sort?

Jack G. Ganssle is a lecturer and consultant on embedded development issues. He conducts seminars on embedded systems and helps companies with their embedded challenges. Contact him at [email protected]. His website is www.ganssle.com.


The book is only free in the USA.

$9 is not much, but it is annoying.

This is not a smart way to encourage non US customers.

- Colin Smith


I have not read the book (yet), but I always have a lot to say on this topic. I agree that Fagan inspections do not fit all environments well. In fact, I would argue that the fatigue and repetition in such a process actually makes reviewers less likely to catch more subtle problems.

However, I don't like the idea of making functions fit on one page. Breaking up complex modules, such as large finite state machines, is more a more complex task than keeping the definitions handy, and the resulting code may be nested in such a way that the code complexity goes up.

I do like "smaller" functions, and in a review I'll always note blocks I think should be pulled out. In C++ I like definitions in blocks they are used in rather than at the top.

Also in addition to the formal review that captures metrics, I am a huge advocate of pulling in an expert (e.g. in my case a software security expert, or maybe a performance expert) to look at the code for certain characteristics that a general review might not find. This can be a very informal process.

And finally, even more than code reviews I find a great way to cut down on defects is to have someone other than the author create unit tests. I have worked in shops that require the unit test to be completed by the author before the review, and that never made sense to me.

In fact, I like the first review to take place with the comments only.

I ordered the book (a great idea to promote their product by the way) and I'm anxious to read it.

- Eric Uner


I've read/heard : "wise teams will insist that all functions fit on a single page".

So to improve productivity, we should rotate our screens 90 degrees.

- Tim Flynn UPDATE TO MY PREVIOUS COMMENT

I contacted Smart Bear and found them to be very helpful, provided prompt answers to my questions and more than happy to deal with a non-US customer.

I was willing to pay the $9 out of my own pocket. The hassle of processing this type of purchase even in a small to medium sized company is just to much of a pain.

Thanks to the people of Smart Bear for their help.

- Colin Smith


Related Reading


More Insights






Currently we allow the following HTML tags in comments:

Single tags

These tags can be used alone and don't need an ending tag.

<br> Defines a single line break

<hr> Defines a horizontal line

Matching tags

These require an ending tag - e.g. <i>italic text</i>

<a> Defines an anchor

<b> Defines bold text

<big> Defines big text

<blockquote> Defines a long quotation

<caption> Defines a table caption

<cite> Defines a citation

<code> Defines computer code text

<em> Defines emphasized text

<fieldset> Defines a border around elements in a form

<h1> This is heading 1

<h2> This is heading 2

<h3> This is heading 3

<h4> This is heading 4

<h5> This is heading 5

<h6> This is heading 6

<i> Defines italic text

<p> Defines a paragraph

<pre> Defines preformatted text

<q> Defines a short quotation

<samp> Defines sample computer code text

<small> Defines small text

<span> Defines a section in a document

<s> Defines strikethrough text

<strike> Defines strikethrough text

<strong> Defines strong text

<sub> Defines subscripted text

<sup> Defines superscripted text

<u> Defines underlined text

Dr. Dobb's encourages readers to engage in spirited, healthy debate, including taking us to task. However, Dr. Dobb's moderates all comments posted to our site, and reserves the right to modify or remove any content that it determines to be derogatory, offensive, inflammatory, vulgar, irrelevant/off-topic, racist or obvious marketing or spam. Dr. Dobb's further reserves the right to disable the profile of any commenter participating in said activities.

 
Disqus Tips To upload an avatar photo, first complete your Disqus profile. | View the list of supported HTML tags you can use to style comments. | Please read our commenting policy.