Title: Professionalism in programming Part 4

Author:

Date: 06 September 2000 13:15:40 +01:00 or Wed, 06 September 2000 13:15:40 +01:00

Summary:

Body:

How do you learn to be a good carpenter? You become an apprentice to someone. You watch how they ply their trade, and listen to their advice as they watch you work. You do not jump in feet first without any practical ability and expect to be able to churn out quality furniture straight away. We do not have an analogue of that in the coding world, even though it is about as much a craft as it is an engineering discipline.

Code reviews, however, do give us a little bit of that in the discipline of an engineering process. I hope that does not put you off - really!

Performing code reviews in a software company is similar in some ways to the model of open source software development. By performing reviews, you encourage responsibility for what you are coding. When you know that your code is not just for you to look at, but it will be viewed, used, maintained and criticised by others your approach tends to change. It prevents you making that 'quick change that you'll fix later' that in your heart you know you will never have time to come back to. The accountability brought on by code review brings a higher quality to coding.

Code reviews are used in traditional software engineering models. They are arguably less important when pair programming, or more than one person is responsible for parts of the code. Nonetheless, they are still useful in these situations.

What are they?

As C/C++/Java programmers, we write code. At the lowest level, we are merely writing declarations, definitions and statements. From these syntactic constructs, we form some kind of semantics in each block of code. At some higher level, we have implemented a particular design by expressing it in this code.

The code review seeks to analyse the section of written code at several levels - the actual design, the expression of that design in the code, and even each individual statement to ensure its correctness. The aim of this analysis is to improve the code, largely by finding and removing faults.

According to Humphrey, "Students and engineers typically inject 1 to 3 defects per hour during design and 5 to 8 defects when writing code. They only remove about 2 to 4 defects per hour in testing but find 6 to 12 per hour during code review" [PSP]. Code reviews are a worthwhile activity!

A code review should not take the place of the code's functional specification review. The code is checked to conform to its functional specification, but the content of the functional specification is taken to be correct. If it were not then the task would be Herculean!

Why do you do them?

We need to be confident about the quality of our programming, and need to find faults as early as possible in the development process. The earlier we try to find problems, the more we are likely to find and fix.

So we perform code reviews to improve code quality. This includes:

removing coding errors,
removing redundant code,
ensuring efficient^[1] algorithms are used

In doing this we also check the code against a number of yardsticks, which may include:

the specification for the work,
any company coding standards,
the appropriate language idioms (e.g. ratify the use of design patterns)

Apart from the obvious benefits that come out of this there are other useful side-effects of code reviews. The cross fertilisation that comes from getting programmers to look at each other's code ensures that the coding style is more uniform across the whole company. It also spreads knowledge about the workings of particular bits of code, so there is less risk of loosing information when people leave.

Many people shy away from a code review for fear it will expose their inadequacies. We should not do this. Having your code reviewed is a good way to learn new techniques. You must be humble enough to admit that you are not perfect and willing to accept criticism from others. A programmer's coding style will improve as they learn from the changes made to their work.

This also means that when criticising code in a review, we must be sensitive. It is interesting that even the least experienced programmer will usually have something worth mentioning in a code review. Moreover, just as the author learns from the review, so may a reviewer.

When do you do them?

In the ideal world every bit of code written is reviewed carefully prior to release. According to the Software Engineering Institute at the Carneige Mellon University, a thorough code review should take at least 50% or more of coding time [SQP] (although they do include 'personal' code review in this statistic).

If we do not have time to review every bit of code, how do we decide whether to review and what bits of code to review? To put is bluntly, you would be stupid not to perform code reviews at all^[2]. Compare it with product testing. You would be stupid not to test a product prior to release. The code review tends to weed out redundancy and bad design more than incorrect behaviour, but certain inexplicable 'features' would be weeded out at this stage which would be practically impossible do diagnose from a system usability test.

Often you will hear people say 'but the code's too large to review fully' or 'it's too complex, no one person could ever understand it'. If a company can employ enough people to write a large program, they can employ enough people to review it. If the code is too complex, it desperately needs reviewing! In fact, it probably needs something a little more drastic. Well-written code should be easily decomposed into sections that can undergo separate reviews.

How do you do them?

In code reviews, we look at code under the microscope - really aiming to criticise^[3] it. This is not to get at the programmer but to improve the quality of the product/software the company produces. Having a code review is not enough. In itself, it is not going to solve all the problems. You also need to make sure that you review properly.

A code review is a formal meeting. There should be a fixed agenda (to ensure no action is forgotten) and a defined ending (not a time limit, as such, but a definition of exactly what bits of code you are reviewing, and which bits you are not - it is very easy to be woolly about this).

An example of a procedure for how to conduct a code review is described below.

Where?

The best place to hold a code review is in a quiet meeting room. The reviewers should not be disturbed. There should be coffee.

When?

Obviously, at a mutually convenient time. Common sense tells us that Friday at 4 p.m. is not a good time.

Roles and responsibilities

One of the most important contributing factors to the success of a code review is who attends. Obviously, the person(s) who wrote the code should be there. The reviewers should be carefully picked. For instance in a recent review at my company, the writer of a library that was used by the author was present. He was able to diagnose incorrect use of the library API that may have otherwise gone undetected.

There should be an appropriate number of experienced software engineers present. There should also be a representative from the QA/testing department.

In addition to the author and reviewer roles, there are a number of other roles for which specific people should be assigned (note that they can be both a reviewer and another role).

Chairman. This person should lead the review, and guide the discussion. They should ensure that the conversation keeps to the point and the meeting does not get sidetracked. Any minor issues that do not need to be discussed in the meeting should be quickly taken off-line by the chairman.
Secretary. The secretary takes the minutes. This means writing down all points that arise to make sure that nothing is forgotten after the review. The secretary role should not be fulfilled by the same person who acts as chair.

Before arrival, everyone is expected to have familiarised themselves with the code. Everyone should have read the supporting documentation^[4] (any relevant specifications etc). Whoever organises the meeting should point out these documents to prevent misunderstanding.

What should you do?

The agenda for the meeting should be as follows:

The author takes a couple of minutes (no longer!) to explain the purpose of the code, and a little bit about its structure. It's surprising what misunderstandings can be caught at this first stage!
Structural design comments are invited. These are comments relating to the structure of the implementation - not the actual code at statement-level.
General code comments are invited. These may relate to a consistent incorrect coding style.
The code is carefully stepped through a line or block at a time to ensure correctness. Things to look out for are described below.
A number of example scenarios of code usage are considered, and the flow of control investigated, if this was not already necessary to understand the code.
When the review has finished the follow-up step should be agreed. The possible scenarios are:
- OK. The code is fine, no further work is necessary.
- Rework and re-review. The code needs a lot of rework, and another code review is deemed necessary following this.
- Rework and verify. The code needs some rework, but another code review meeting is unnecessary. Someone is nominated to act as verifier and when the rework is completed, they check the work against the recorded minutes of the code review meeting.

A reasonable deadline should be imposed for any rework (so that the reasons for actions stay fresh in people's minds).

This is a general framework for a code review meeting. It's beyond the scope of this column to describe what 'good structure' is for a piece of code. However, there are a few elementary points we can draw out that are worth noting. The reviewed code needs to be:

Predictable. The code should not be self-modifying, reply on no 'magic' default values, and not have the possibility of infinite loops or recursion.
Robust. Wherever possible the code should protect against detectable run time errors (divide by zero, number out of range errors, etc). Input parameters should be checked, for both function parameters and program input. C++ code should be exception safe. All appropriate signals are caught.
Data checking. Bounds checking is performed on array access, and other similarly insidious data access errors are avoided. Multithreaded code has correct use of mutexes. For any system/library calls the return value is always checked.
Well structured. The implementation design is correct. The code is easy to understand. There is no redundant code (any obvious cut-and-paste programming, for example).
Complete. The code implements the entire functional specification. It has been shown to have been integrated and debugged satisfactorily. It passes all test suites.
Correct. The code meets all relevant standards and its requirements are met. All variables are of the correct type (there is no chance of overflow). Comments are accurate. For embedded platforms, code meets any memory size requirements. There is appropriate use of libraries, and all function parameters that are passed are correct.
Maintainable. The programmer has been wise in their use of comments. The code is kept under correct revision control. There is appropriate configuration information. The code formatting meets 'house standard'.

The use of tools

There are a number of different tools that may be useful when performing a code review. For example, there are tools like the McCabe toolset [McCabe] that help trace flows of execution, and calculate values for function 'complexity'. This last metric is very useful when identifying which pieces of code need reviewing as soon as possible. A visual design program may aid in understanding the code structure and its dependencies (particularly in object oriented languages like C++ and Java).

Conclusion

Code reviews are an essential part of the software development process. Just as an apprentice learns their trade from knowledge passed on, code reviews spread knowledge and teach coding capability. As more of a peer-to-peer than master-apprentice activity, they provide a learning opportunity for author and reviewer alike, and result in higher quality code.

In the light of code reviews, you should write your code bearing in mind that it is not just for you to read. Other people must be able to maintain it as well. Authors are accountable for the quality of their code.

References

[PSP] Humphrey, Watts S. Introduction to the Personal Software Process. Addison-Wesley, 1997. ISBN: 0201548097

[SQP] Humphrey, Watts S. The Software Quality Profile. URL: http://www.sei.cmu.edu/topics/publications/articles/quality-profile/old.file.html

[McCabe] McCabe and Associates. McCabe IQ. URL: www.mccabe.com

^[1] For an appropriate definition of 'efficient' in it's context.

^[2] There are plenty of stupid companies out there.

^[3] Sorry, verify.

^[4] Naturally, all supporting documentation will have been thoroughly reviewed beforehand…

Notes:

More fields may be available via dynamicdata ..