This page was originally contributed by Michael Pruemm. It was written by a colleague of his; Michael’s comments are in green.


After many reviews of code of colleagues I would like to share my experience as reviewer and give some practical advice on reviews. I hope that my pragmatic approach can help to improve the quality (and quantity) of reviews. Maybe this advice can evolve to some guideline or handbook for reviewers.

Reviewing is more than a supplement to testing.

In fact, it should occur before significant time is spent on testing, because you can find many otherwise hard to test/debug problems early.

Maintainability, extensibility and scalability are aspects of an application that cannot be tested. They can be checked in a review. The structure of an application can be checked in a review, not in a test. Some problems like incorrect exception handling and synchronization are hard to find in tests and difficult to debug. A reviewer can find them.

And above all: The one doing the review and the programmer of the reviewed code learn from eachother. It will make a better programmer of both of them.

Properties of a good application

(between square brackets the aspect that realizes it):

I am wondering whether there is an ordering for the things within the brackets. I would attribute simplicity first of all to the design, than to the structure.

But this is also a property of the design. E.g. the uniform handling of Init/Terminate in the DVD projects.

... and to a lesser extent design, too. Think of minimising the coupling of components.

The low level aspects of good code can be captured by style guides and style checking tools. The high level aspects of good code, the logical

I would say “Some low level aspects...” and “Many high level...”

and functional correctness, can be checked by system tests. The aspects in between can be described as the structure and design of the code. The design is (or should) be well documented and contains the strategy to realize certain requirements of the system, such as algorithms, fault tolerance, security, performance and interfaces. They are generally well

Much more import is to document any design decisions that were made and WHY they where made. Remember the (very) short “design guidelines” I made up for the DVD1 project? I should have them somewhere at home, I should go and find them. They should be developed a bit further in the same style as your review notes here.

discussed with designers and architects. Between design and coding style is an aspect that I would call the structure of the code. The structure is left to the individual programmer and is seldom discussed, poorly documented and not tested. This is a weak point and even a risk in projects. Why? Because an application with a good structure is:

Should be: “to show TO your customer”

This list somehow reminds me of what Dijkstra called “elegance”, but that included much more, among other things brevity of expression etc.

The structure of the code encompasses:

Although it’s hard to give proper definitions for structure and design I think that most programmers know what I’m talking about.

One of the most important jobs of a reviewer is to check the structure of the code. A second aspect to check is the use of the language and programming environment.

A checklist for the reviewer:

This list should come with specific questions... e.g. Are there any comments? Are they helpful? Are they correct (do they match the code)? Are they too wordy?

.......... more to come.