* 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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Mailing list: https://launchpad.net/~ayatana-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~ayatana-dev More help : https://help.launchpad.net/ListHelp
