Make Reviews Constructive Not Caustic
Make Reviews Constructive Not CausticA concern is that CodeReviews will become intimidating and generally unpleasant, not to mention unproductive, if they are not done carefully. To ensure that the programmer(s) who wrote the code under review do not feel under siege, some CodeReview guidelines are in order.
PairProgramming is essentially a continuous source CodeReview process. It might be less threatening because problems tend to get caught at the "microgoof" stage. Also, it seems like a less adversarial environment overall. -- JohnBrewer
Using CodeReview, or reviews in general as a learning activity is in my experience a fatal idea. It is just the wrong time and place. Reviews are for ensuring a certain quality, not for teaching stuff. It's not about knowledge transfer to people.
For a review you need people who can "judge" on the subject at hand. Calling it a learning activity gives far too many people an excuse to attend, and to attend unprepared. This then becomes a boring waste of time. You need few, but highly skilled reviewers, you need previously well defined criteria to do the review (remember this requirements documents you might have? :-)), and the reviewers all(!) have to be prepared, or the meeting is doomed.
And you have to brainwash people to get rid of the assumption that reviews are for fixing things. They are not. Reviews are for measuring how good or bad something is ("good enough" for your task at hand?). As a small side effect, obvious things are pointed out and fixed. In general, however, the review of something after it had been produced is far too late to fix things if they are really wrong. You should better have done that earlier in your process.
Only once have I been on a project with pretty good reviews. We broke nearly all the rules, however. We met regularly, and often had to scrounge to find things to review. So, our reviews were not a measure of whether code was good enough to go to the next phase of its life-cycle. In fact, we didn't keep very good statistics of our reviews, though we did make ToDoLists. We had 7 or 8 people in the reviews, and the education value was as great as any other. Code was all PrettyPrinted so we didn't argue about CodeFormatting rules. We had a style checker that we programmed to check more and more things automatically. When we got tired of seeing certain kinds of errors, we'd try to add some rules to the style checker to spot them. I don't know whether our process worked better than the standard one, but I know that we liked it. -- RalphJohnson
My experience is on the same side as Ralph's. Notions of "judgement" seem rather counter-productive. When we do CodeReviews, it is for polishing (read, fixing) the code, and spotting errors. Discussing (and learning) language idioms and elements of good style from each other is essential and valuable aspect of the process, too. There is no better opportunity to teach someone to (for example) refactor long methods than sit down with him, read his code and say "OK, this place looks a bit difficult to understand, besides you have these 10 lines almost same here and here. How about we call them a method? See how it plays to make YOUR code more elegant?".
Another striking similarity is the use of PrettyPrinter and style checker. It is useless to review formatting - it doesn't matter what it is, as long as it is consistent and the left margin reveals structure of blocks, while the right margin is visible on the screen. PrettyPrinting makes all that happen for free (even though it is not the cutest possible formatting). So, the first thing I usually do if I see a messed up indentation, is to say the above phrase and press Shift-Ctrl-F, Ctrl-S (autoformat and save in Eclipse). Style checkers are mostly waste of time, unless you deal with formatting warning either by pretty printer or by switching all these warnings off in the stylechecker configuration.
One difference with Ralph. We do it in small groups (2, sometimes 3 people). -- AlexeyVerkhovsky
EditText of this page
(last edited November 7, 2014)
or FindPage with title or text search