cweir@marlow.uucp (Charles Weir) (06/26/89)
Suggested approach for a Code Walkthrough
Following the discussion about code reviews, here is a document I put
together a little while back. I should be interested to find out how
it compares with other people's experiences.
We had been using code walkthroughs to find bugs in code where
testing was proving unsuccessful. Sometimes they were very
successful, sometimes not. This note attempts do analyse what leads
to a successful review.
Our targets for a walkthrough are:-
* No Bugs.
* No indeterminate situations.
* Maintainable and understandable code (as for as convenient) .
A bug is a straightforward point where under some situation the code
will not work as the programmer intended. An "indeterminate
situation" is where the code behaviour is unpredictable (technically
this is a bug too).
How we go about it
------------------
We used the following guidelines:-
A code walkthrough is constructive. The walker needs to avoid
attacking or threatening the programmer either directly or indirectly.
The walker must focus attention on a particular problem, a particular
line of code or element. We avoid issues of approach and style ("I
wouldn't do it like this..."), or unnecessary performance enhancement
("It'll be faster if...").
We tackle problems using questions rather than statements:-
* How Is this function defined?
* How does this work?
* What parameters are being passed here?
* When is this called and by what?
* I think I may see a problem here. Am I right? (N.b. Quite
often the answer will be no)
The process of questioning the code is more effective than a straight
challenge. It is non-threatening. And it encourages the programmer
to look at his/her code from a different perspective.
The inspections should not last too long. The mental effort
required is similar to University entrance examinations, so
over-stretching here oneself does nothing for anyone. A sensible
maximum is two hours for a session or less. It is much better to
inspect a body of code bit by bit in a number of short inspections
over a week or two, than all at once.
Further Suggestions (particularly for C code reviews)
-----------------------------------------------------
Bug hunting too is partly a matter of habitat. It is important to
look at the places where bugs live. We look mostly at:-
* Buffer, string, and arrays. Could there be overruns? Zero
termination of strings? "strlen" etc.
* Error Handling: Is an error ignored? Does the handling
mechanism work?
* Statuses: Are there states (combinations of statuses and flags
etc.) which can occur but are not handled? Or which are handled
but could never occur?
* Double use: Same data structure used by conflicting processes.
Flags and status variables used for two purposes.
* Comments: Misleading or out-of-date? It is often worth
covering up comments when inspecting the code.
For the inspection to be effective, it is not necessary for the
reviewer fully to understand the code and the design. The
questioning process uses the programmer's knowledge to supply that
information.
It is important to keep a sense of perspective. In C, for example,
it is important to replace magic numbers with "#defines" for
maintenance purposes: This is not the same as a bug. Function
headers and comments generally are necessary for future maintenance:
Again these are not bugs. And we shouldn't treat them as such.
The GOTCHA trap is a classic: It's easy to communicate the sense
of victory at finding a difficult bug in a way which threatens the
programmer ("GOTCHA"). Again we use questions to communicate,
rather than statements. A sense of humour is important too. A laugh
both defuses the tension, and makes the process more fun.
--
Charles Weir, Reuters Limited, 85 Fleet Street, London EC4P 4AJ
Tel: +44+1+324 6231
cweir@marlow.uucp OR cweir%uucp.marlow@idec.stc.co.uk
{backbone}!mcvax!marlow.uucp!cweir