Fog Creek Software
Discussion Board

Code Review Documentation

This is an addition to the earlier Code Review thread.
I just got hired as a contractor to do code review for a Palm medical application. The entire development was done by an outside company, which has completed its work. The application is bug free(as far as we know!) and runs fine. But the company that hired me wants to make sure everything is in order.
They want me to submit a documenation/report and I wanted to find out from you guys what kind of documention should I provide?
I have done quite a bit of software testing and extensive software development, but nothing like this. The people who hired me are medical professionals(non-technical). So should I provide a techincal or non-technical documention? What topics/modules this document should contain? Thanks in advance for all your inputs.

Wednesday, December 11, 2002

A code review is first and foremost a search for bugs.  The meat of code review documentation should therefore be simply reports of individual bugs.  To be 'formal' you need to document *what* was reviewed (i.e. what files, preferably specifying the version).  So, in the absence of defect tracking software, but in the presence of source code control software, your document could state (this would be for a comprehensive review of an entire software product, done all at once, which is not all that normal):

1.  The scope of the review (like, for example, the software source that is required to build blah.exe)
2.  The software files and versions, e.g.
  foo.cpp, v1
  bar.cpp, v3
  widget.cpp, v7
  gadget.cpp, v1

3. On a per-file basis, the bugs found.  Probably in a nice table.  Perhaps with recommended remedial action -- for code-review discovered bugs this is usually pretty easy.  Category and severity could be recorded -- i.e. potential dereferencing of a invalid pointer is "fatal", a potential memory leak is "severe", a misspelling that will show up in the UI is "cosmetic", or something like that.  You would, of course, lay out all the different severity levels before you launched into the review, and document what you mean by them.

In the presence of defect tracking software, it would make more sense just to enter the bugs into the bug-tool.  But you'd still need the document to point out what you reviewed, and perhaps summarize your findings, and point out to readers how to look up the bugs in the bug-tool.

You may also want to point out other issues beyond simply bugs.  Maintainability issues, etc.  But bugs are the main priority -- unless you've received a different mandate from the client.

Wednesday, December 11, 2002

It should be both tech and nontech because you need to be able to convey to the laymen the consequences of the bugs you find and you need to be able to convey to the future engineers (I would imagine, potentially including yourself) where the bugs are and maybe how to fix them.

Wednesday, December 11, 2002

Both in the same document.

Tech stuff in font color sky blue, other non-tech stuff in black.

Prakash S
Wednesday, December 11, 2002

> A code review is first and foremost a search for bugs.

I do not neccessarily agree. There are other very good reasons to review code:

learning from each other

trying to find a common "style" in programming, well readable and maintainable for all programmers working together on a project

finding possible sources for trouble (before they become actual bugs) like undefined behaviour etc.

What you want to document about a code review depends on the reasons you reviewed the code in the first place. To avoid buerocratic overhead, you might decide that the reviewed (and in some placed changed) code with some comments (like: "XY and ABC decided that a typedef is needed here") either in the code itself or in your source control system, are enough. No need to create tons of new documents for that.

Have fun,

Jutta Jordans
Thursday, December 12, 2002

"> A code review is first and foremost a search for bugs.

I do not neccessarily agree. There are other very good reasons to review code:"

The assertion that a code review is first and foremost a search for bugs in no way implies that there aren't other good reasons to review code, so I don't understand precisely why you disagree.  Is a search for a 'common style' or 'learning from each other' more important than finding bugs?  More importantly, if the time spent on the code review comes out of the project budget, which it usually does, what do you think is more important to the project?  Finding bugs, which has a direct impact on the success/failure of the project, or 'finding a common style', 'learning from each other' etc.

I think one of the reasons that code reviews *fail* is the failure to realize that code reviews are a defect identification mechanism.  When they become a forum for battles over style, they become a burden, rather than a boon, to a project.

Thursday, December 12, 2002

"The people who hired me are medical professionals(non-technical). "

Nevertheless, they must have a goal in mind for the contract deliverables.

Since they are paying, you must find out what that is.

Then you can apply your expertise to supplying something that fits the bill.

I realize that this carries a slight risk of them saying "Mmm, we don't really know, perhaps we don't need it after all", but even that is better than mutual discontent at the project's end (isn't it?).

Non-recoverable hours of your life will go into this - I would want to know they would count for something.

Attending the wake
Thursday, December 12, 2002

*  Recent Topics

*  Fog Creek Home