Code Reviews

From Apache OpenOffice Wiki
Revision as of 09:01, 29 March 2007 by Np (Talk | contribs)

(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

Varieties of Code Reviews

In Code Reviews one or a few persons read some source code and try to find pieces that could be improved. Depending on goal, topic, means, frequency and number of persons involved there exist various types of Code Reviews. A few examples are:

Fagan Inspections
See http://en.wikipedia.org/wiki/Fagan_inspection
This process is very formal, needs lots of time and several people. Fagan Inspections try to find as many defects as possible and to correct them immediately.
Complete Code Reviews, covering all code of a project
This is practiced for example by the Mozilla community. Every code is reviewed by another developer before committed into the source code repository. Can be only one or a few reviewers. Goal is still to find as many defects as possible immediately.
Lightweight Code Reviews
Need only one reviewer. The main goal is learning through the process of communication and of revisiting code consciously. Finding all defects at time of review is no priority.
Self Reviews
The author herself reviews her own code a while after writing it. Preferably there should be at least a night inbetween. Needs no other person. While this kind of review finds less defects and triggers less learning than reviews where other people are involved, they still can be quite valuable. Some defects are only found by the author, because she understands best what is going on.

Means and Terms

Rule

A Code Review needs rules to decide what is undesirable, what should be improved and what is okay. Rules could be quite general (like "The code has to be understandable."), but need to be specific enough that a reviewer can decide, if a rule is met or not. If there are no rules, but reviewers decide only by personal intuition, the outcome of a review is too unclear.

Rules regarding source code may cover various aspects:

  • correctness
  • avoidance of dangerous constructs
  • maintainability (readability, clarity, ease of change)
  • compliance to local conventions of a project
  • portability
  • ...

An example rule set are the Cpp Coding Standards of OpenOffice.org.

Defect

Definition: A defect is a violation of a rule the participants of a review had agreed on.

This does not necessarily mean, it is a functional bug, but can also be a matter of clarity or conventions. If code that is obviously quite well, shows a defect, this can be a hint the related rule needs to change. But it would still be a defect, as long as the rule is not changed.

Author

The person that wrote the code or changed it. It makes sense, if the author reviews her code herself simultaneously with the reviewer(s), because often just by re-reading the author gets new insight. The author shall never be criticized in reviews. Not the author is reviewed, but the code!

Reviewer

A person that reviews code. Typically not the author. Reviewers need the skill not to get into discussion with the author while reviewing, but to just observe the code.

The reviewer can be a person with problem domain expertise or without, both has advantages. A domain expert is more likely to spot domain-related bugs. But a person from outside may even have a clearer view on general problems or questionable habits that have been introduced into a team.

Personal tools