Weekly Edition
Return to the Development page
| |||||||||||||
Humphrey: On Code ReviewAt his blog, Mozilla's David Humphrey discusses the project's use of mandatory code reviews. "I’ve had people tell me that so-and-so is spending too much time on reviews, implying that if they were writing code instead, they’d be more productive. The thing this misses is how one developer can only write so many patches, but their influence and ability to affect many more is increased through reviews." It is easy to work alone and keep the project small, he says, but reviews are critical for growth. "Code review is humbling, and it’s good to be humbled. Code review is educational, and it’s good to be taught. Code review is public, and it’s good for open source to happen where people can see what’s going on." (Log in to post comments)
Humphrey: On Code Review Posted Jan 21, 2013 15:47 UTC (Mon) by dps (subscriber, #5725) [Link]
It can be very difficult to fix things.esp. if they come from third parties.
A large SAN vendor, who shall remain nameless, provided a command line tool where 0, 255 (-1) and 2 all mean various sorts of failure and 1 success. It should not require code review to limit the numbers passed to exit(3) to 0 to 255 and make success exit status 0.
However we support issues meant that instead of fixing this the code that uses this tools works around it: when calling this particular program non-standard exit code processing knows that 1 mean success,
In a similar vein if an existing program can do something at a push it might be preferred to a new one that does exactly what is required. Let it suffice to say that one large commercial product is affected.
Humphrey: On Code Review Posted Jan 21, 2013 20:02 UTC (Mon) by JoeBuck (subscriber, #2330) [Link] Perhaps I'm missing something, but isn't that the kind of thing code review is supposed to catch: reviewers reject the original code and require either that the new code be fixed to follow the standard error convention, or if the third-party code can't be changed, to always call it via a wrapper so the translation of error codes happens in one place?
Humphrey: On Code Review Posted Jan 21, 2013 16:56 UTC (Mon) by NAR (subscriber, #1313) [Link]
I remember that about a decade ago code review here meant that people printed copious amount of code to dead tree, disappeared into a meeting room for hours, then emerged in a foul mood (the result of bickering about the necessary number of spaces) - and then nothing happened with the results, because the code review was scheduled only after delivery, due to everyone being busy with last minute fixes. All this made the "code review" expression dreaded.
In recent projects we've simply setup the infrastructure to send an e-mail about each commit, so the interested parties can review it (hopefully in time). It is not mandatory, but I think a very useful tool not only to spot errors, but to keep up to date with remote parts of the codebase.
Humphrey: On Code Review Posted Jan 21, 2013 17:07 UTC (Mon) by marduk (subscriber, #3831) [Link]
I too remember a similar process (nearly two decades past) whereby all code was printed and had to be reviewed by a senior-level person. If the submitter were also senior level (or happened to be particularly friendly with the reviewer), then she probably didn't even look at the code and quickly signed it off. If the submitter were at a lower level (or an advesary of the reviewer), the code might be meticulously picked apart. There were no safegaurds to ensure that the reviewer was actually fit to review the code in the first place other than she had been with the company long enough to get "senior" status.
Humphrey: On Code Review Posted Jan 21, 2013 23:53 UTC (Mon) by marcH (subscriber, #57642) [Link]
Enforcing any process or tool is pointless as long as the majority fails to understand the value of it. A process should only (try to) organize things people genuinely want to put some effort in.
Finding time for code reviews is hard enough when people agree they are useful; no surprise they're not done properly when people miss the point.
If you work in a company that does not even understand the value of code reviews then you should probably try to find a better job where quality, teamwork and personal development actually matter. Run even faster if a rigid process forces code reviews to happen anyway.
Humphrey: On Code Review Posted Jan 22, 2013 2:02 UTC (Tue) by rgmoore (✭ supporter ✭, #75) [Link] Enforcing any process or tool is pointless as long as the majority fails to understand the value of it. A process should only (try to) organize things people genuinely want to put some effort in. I'm not sure if I completely agree. Sometimes people have to experience something to appreciate the benefits, so enforcing it as a matter of policy has to come before general acceptance. That is admittedly a high risk approach. It will only work if the people implementing the new policy understand the benefits and do a good job of enforcing it in a way that will eventually make the benefits clear to everyone else. Simply trying to enforce an idea, even a good idea, without a clear understanding of the way it's supposed to work is asking for failure.
Humphrey: On Code Review Posted Jan 22, 2013 9:16 UTC (Tue) by ibukanov (subscriber, #3942) [Link]
In Mozilla model there are several people who can do a review for a particular component. That has drawbacks.
When I used to work for Mozilla one of problems with reviews was to discover who at a particular moment can really review the code as opposed to just enforcing a code style. But people who do the valuable reviews spend more time on that and their queues of patches to review grow so one have to wait days or even weeks before getting a review. And with a review queue there is temptation to start rubber-stamping patches as opposed to doing real reviews.
Another drawback of multiple reviewers is that as the code for their component growths the reviewers would have only vague understanding of the codebase they can review. They simply do not follows all the development. For example, before the Tracemonkey JIT for JavaScript was removed from the source tree, nobody really new all the details of how it worked and implications of changes.
I think for Mozilla it would be worth to try to implement the Linux model when all patches for a particular component has to go via one person. Then there is a chance that at least one person does understand the code.
Reviews are great... Posted Jan 23, 2013 20:36 UTC (Wed) by knobunc (subscriber, #4678) [Link]
With the right tools... We're using Review Board at work and it has made reviews easy to do, and useful. It has a great mode where you can see the diff off the diff to see what changed between versions of a review.
It's MIT licensed, and relatively easy to install.
|
|||||||||||||