[Ayatana-dev] Reviewing code, why and how

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

* These comments are my opinion, gained through experience on the Launchpad 
team and prior working experience.

Reviewing code provides a number of benefits to all involved:
 - The code itself gets new eyes on it, and we all know that with enough eyes, 
all bugs are shallow [1]
 - Making sure that the new code follows the project coding guidelines
 - Making sure that the new code fits with the design of the project
 - Spreads the understanding of the code from the author to the reviewer(s)
 - Discussion around the code between the author and the reviewer aids in the 
understanding for *both* the author and the reviewer


Getting your code reviewed

For the ayatana projects, we use Launchpad.  The first step is obviously to get 
your work onto launchpad itself, and propose the code for merging.  A really 
important thing here is to realise that proposing your code for merging 
shouldn't be the first interaction that you have with the other developers 
about the work that you have done.  I say this for a number of reasons which I 
don't want to go into in too much detail right now as it will make this email 
epic, however the primary reasons are to make sure that you are not going to 
waste your time.

On the Launchpad team we had a process called the "pre-implementation call".  
This is where you chatted either with VOIP, or on IRC about what you were 
going to do to fix the bug (assuming that you are working on a bug).  For 
simple bugs, it was a very simple call (or chat).  For more complicated work 
the pre-implementation call worked very well as an initial sounding board and 
simple design phase. [2]

A key part to the merge proposal is the description.  You shouldn't assume 
that the reviewers have read the attached bug reports in any huge detail.  
Often there are subtleties that are missing from the bug report description, 
or are noted in comment 16, which as the author of the fix you may know, but 
the reviewer may well not know.  In the description you should really say the 
following:
 - What is the problem being fixed (even in a few words)
 - What you did to fix the problem, and why that fix is the right one (for now 
[3])
 - Which tests you need to run to illustrate that the problem has indeed been 
fixed.  Ideally these are unit tests, but could also include steps to reproduce 
the old behaviour, showing that the bug is no longer occurring.


Reviewing Code

Reviewing code should be a conversation.  There are rare occurrances where the 
code produced is perfect (or at least very good).  It does happen, but it 
isn't really the norm.

There are several levels of review that a reviewer can do.  Each takes more 
time than the previous level
 - Pure syntax and coding guidelines - these are very mechanical reviews
 - Reviewing the test coverage, is the code sufficiently covered by 
understandable tests
 - Readability and understandability - is the code understandable.  Is it 
obvious to the reader of the code as to what is happening and why
 - Design reviews - is the code factored correctly.  Should things be broken 
apart, or merged together.  Is the code following good design principles?

Reviewing the code for correctness shouldn't really be part of the review, 
primarily because the tests should illustrate the behaviour and should be 
obvious in that demonstration of correctness.

Dealing with tests, particularly in unity and nux, is an interesting target as 
we attempt to get more of the code covered with good unit tests.  We should be 
at least producing our new code with test coverage.

A key part for reviewing code is "if in doubt, ask a question".  The merge 
proposals in Launchpad handle comments very well.  Also, if changes a needed, 
and new revisions pushed, you don't need to create a new merge proposal.  
Launchpad will update the diff on existing merge proposals, and the comments 
will show new revisions pushed between comments.

Don't be afraid to ask a question, and expect questions to be asked of your 
own merge proposals.

Tim
thumper on freenode


[1] http://www.goodreads.com/quotes/show/160674

[2] I'll stop there now.  If people are interested, I can write more on this 
topic later.

[3] Sometimes it is better to provide a temporary fix rather than a complete 
refactoring.  If this is the case, please say so.

Attachment: signature.asc
Description: This is a digitally signed message part.