How to ... review code

Image via Wikipedia

In the last post we covered four fundamental ways to improve your code: read, write, review and contribute. This post will cover reviewing code: how to review, how to be reviewed, what to look for during a review and when to perform a review.


How to be reviewed
The single most important thing to remember during a code review is that the person who wrote the code is not the code. Computer programming is a very personal and creative process, where the programmer creates something from nothing. The result might be highly logical but the process is just as creative as painting or writing and the programmer has put just as much of themselves into the result as any artist or writer. Programmers, especially those new to the craft, often feel deeply attached to their code and feel that any criticism of the code is a criticism of them as a person. Learning to let go of your code can be a painful process but is vital if one is mature as a programmer.

Code reviews are divided into two rough categories: formal and informal. Formal reviews follow a strict process which leads the participants through various stages and while this has been shown to be very effective a removing defects, most programmers balk at the idea of spending so much time in meetings going over code line by line. For situations where programming is not the main aim of the participants, such as programming as a scientist, the overhead of such a process might seem unnecessary.

Informal reviews are, obviously, more ad-hoc and driven more by the individual programmer rather than a lead or manager. For the scientist-programmer this is much more in keeping with more informal development environment. The most common informal review is the 'over the shoulder' review where reviewer sits at the writers desk and goes through the code.

As it is unlikely there will be a formal code review process in place, it will be up to you to lead the process. This gives you more control over who reviews your code and what you choose to do with their input which will reduce any anxiety you might feel about the process. If the people who are reviewing your code are inexperienced then you can help them by providing guidance, and possibly a list of suggested questions, on what you are looking for (see later for specific suggestions).
Here is a list of suggestions for the person being reviewed:

1. Listen, only ask specific questions or clarify something


2. Don't argue or defend your work (or yourself!), you can choose what to do with the suggestions, even ignoring them, but if several people make the same suggestion then it is worth listening to.


3. Take notes of specific suggestions, so you don't forget and so you can clarification or suggestions later.


4. Only accept constructive criticism, if somebody criticizes without providing any suggestions to improve or fix problems, ignore them. But, always ask them for a suggestion first!


5. Be professional, the code isn't you. Get suggestions on how to fix problems/issues and then decide how to act upon the it.

How to review
Congratulations, you have been asked to review somebody else's code. They obviously think you are a person worth listening to and a person whose opinion and technical skills they respect. This is great opportunity to help a friend or colleague improve as well as a way to learn how somebody else has solved a problem.  Here is a chance to learn something new.
The most important things are to be constructive and specific as possible in your criticism/suggestions.

Depending upon when in the development cycle you are doing your review and what kind of person you are, you will ask for different things before the review. If this a design review then you might conduct it at a whiteboard rather than a computer, without any code. If the review is later in the cycle, you might want to see the code first or do two passes: one where the code is explained, and one after you have had time to review it yourself. It is important to agree with the writer what is suitable, they don't want you to sit on a copy of some code for a week as they are working on it, by the time you have reviewed it, it will be out of date.
Here is a list of suggestions for the person doing the review:

1. Give an overview critique first. If you are being taken through the code for the first time ask for an overview before diving into the details. If the code has mostly been written then high level/architectural changes are very costly so are unlikely to happen, if this is the case then phrase your criticisms/suggestions as more things to think about in the future. If you are working at the design stage, when high level changes are still relatively easy, then a more critical line can be taken.


2. All criticism must be constructive. If you find a bug then fix should be given. If the suggestion are more stylistic ('The variable names are too vague') then a better style should be suggested with reasons why it's better ('Because I use it' is not an acceptable reason!).
   


3. Ask questions. Confused code is probably buggy code, if it isn't clear in the code it probably isn't clear in the writers mind either. Help them clear up confusing sections.


4. Remember that coding style is highly individual and unless it is highly inconsistent or very opaque then it shouldn't be criticised. Suggest improvements and explain why they are better.


5. Listen to your intuition, if you have lots of experience, otherwise use logic. As programmers gain experience of well written code (the well written bit is important!) then they will increasingly get 'feelings' about code. Use this intuition to track down problems as you are probably experiencing a bad code smell or the use of an antipattern, investigate further to narrow it down.


6. Never force your solution on the writer. You might not agree with the approach but you are limited to suggesting problems with the current approach and ways to improve it, never try and take over the development of the software!
   

What to look for during a review
All code is written for a purpose and if it fulfills this purpose it can be considered a success. A good review should cover the requirements of the problem, as well as the chosen implementation. It is always good to consider use cases, specific tasks the user will do with the code, as a way of understanding how control and data will flow through the code. If the code doesn't do what it is supposed to then it doesn't matter how well written it is!

Secondary concerns are for more abstract concept such as efficiency, easy of maintenance and easy of reuse and you should be careful when judging code against these criteria. For instance, speculative generality, generalising code before you need to, should be considered a problem in code but having highly coupled code should also be considered a problem. It is up to the judgement of the writer and reviewer to decide where the line should be on a case by case basis. The craft of programming lies in learning what to do when and why.

When to review
As with testing the earlier you can begin reviewing the better the outcome for your code. As with all project there are 3 times when it is good to seek input: the beginning, the middle and the end.
Having a design review is a great way to see if there are problems in the basic approach you are taking to solve the problem. Changes at this stage are cheap so getting a lot of input gives you lots of ideas. The problem you have to avoid with reviewing is making sure the program doesn't become design/write by committee. You, as the programmer, are responsible for your work and on choosing a particular approach to solve the problem. You can get input and feedback but the ultimate decision is yours.

At the middle of the project you have some experience of the details of the problem and will have probably hit a few roadblocks that you hadn't anticipated before you started. A review at this point is a great way of taking a step back and checking you are still solving the problem in the right way. Large scale changes are more costly but might be necessary now you have more details of the problem.
Review at the end of a project is a great time to catch bugs, improve performance (although this must always be accompanied by profiling) and to perform a postmortem on the approach you took.

Summary
Code reviews often inspire fear in the people being reviewed, because they feel criticism of the code is a personal attack, and frustration on the part of the reviewer, as they feel the review is a waste of time and they don't want to annoy the writer. Hopefully we have shown that with a positive attitude on the part of the writer and the reviewer, and a few simple guidelines to get the most out of the process, a lot can be learned by both the writer and reviewer.

Do you uses code reviews? Do they work and what suggestions do you have for others?