Sponsored link
Vyatta –
Linux & Open Source
Alternative to Cisco –
Advanced Routing,
Firewall, VPN, QoS..
Free Download ->
| |||||||||||
Where have the reviewers gone?Where have the reviewers gone?Posted May 24, 2007 3:47 UTC (Thu) by neilbrown (subscriber, #359)Parent article: Where have the reviewers gone? Reviewing patches can be unrewarding work. While I completely agree with this statement, I would like to assert that it can also be very rewarding work. I have had a couple of experiences recently where some review work was quite fun. I think that a key element of enjoyment is the opportunity to meaningfully contribute. If the code is near-perfect, then there is not much room for contribution, and so not much pleasure. On the other hand, if the code is at total mess, then there is not much room for meaningful contribution either: "where do I start...." But there is plenty of middle ground. Code that fits the coding standards of the project and has been tested for a couple of test cases, but probably still has a few warts is the sort of code that can be fun to review. The reviewer can learn something. They can experiment. And they can probably find a few bugs or room for simplification. While the author could probably find those bugs too if they looked a bit longer, letting the reviewer have a chance is just "Good sportsmanship". I think the maxim "Release early, release often" is still good, but to please reviewers I would add "not too early, not too often". You don't want the reviewer to give up in disgust, or to get reviewer-fatigue. I think it is important to have developer/reviewer teams (and yes, we need more of them), and it is important that they are both involved from early on. Getting code reviewed shouldn't be the last step. It should be integral to the development.
(Log in to post comments)
Where have the reviewers gone? Posted May 24, 2007 8:52 UTC (Thu) by nix (subscriber, #2304) [Link] People have commented in the past that if they turn out near-perfect codeit takes longer to get reviewed than if they turn out code with a few intentional warts in it that the reviewer can pounce on :) (obviously if the reviewer doesn't fix them, the submitter can, as they're intentionally introduced!)
Where have the reviewers gone? Posted May 24, 2007 9:33 UTC (Thu) by MathFox (subscriber, #6104) [Link] It very much depends on the review-instructions. I have done code reviews on various (proprietary) projects and on one project we had an pretty simple and effective review guideline:Would you accept the responsibility to maintain (fix bugs) in the code?The goal wan not to get 100% bug free code, but to get clean and fixable code. (If we saw flaws, we pointed them out; most of them were then quickly fixed.) I can imagine that PostgreSQL places the bar a little higher and looks for data integrity and security issues too. Anyway, it is reasonable to expect a peer review to take 5-10% of the coding time; when you need more, the code has become too complex and is ripe for a (re)design.
Where have the reviewers gone? Posted May 24, 2007 13:56 UTC (Thu) by nix (subscriber, #2304) [Link] Oh dear, that wouldn't work very well with me, or with anyone else who has a chronic inability to say no :/
Where have the reviewers gone? Posted May 24, 2007 19:28 UTC (Thu) by smoogen (subscriber, #97) [Link] Well when we dealt with it.. we stuck the person who said yes until we either realized that their input was useless or they really meant that they would support the code. The opposite also showed up. If a person constantly said no because they didnt want to do the work later of supporting it.. they found that their votes were less valued (or asked for).
Where have the reviewers gone? Posted May 24, 2007 23:46 UTC (Thu) by giraffedata (subscriber, #1954) [Link] I think that's a very vague standard. I myself would pass just about any code under that standard, because if I were to have responsibility for supporting it, I could just rewrite it all to meet my requirements. I have in fact accepted responsibility for supporting code (often for money) that I thought was poorly written and had various apparent bugs.And on the other hand, I would hate to have someone review my own code under that standard, because a reviewer can interpret it as meaning if my code isn't written the way he would do it, I can't ship it. I.e. I must function as some stranger's lackey. A clear definition of the meaning of a review is essential to eliminating the arguments, frustrations, and hurt feelings that turn people off of both sides of reviewing (don't want to review; don't want to submit code for review). And yet, I almost never see it. And, strangely, most people don't seem to notice it missing. There is quite a variety of activity that can be a review. One kind of a review is simply advice. You're pointing out to the coder things he may have missed. You've satisfied your obligation if you've taken a reasonable look at the code and communicated every issue that you think he may have overlooked. The code is the responsibility of the coder. On the other end of the scale is acceptance of the code. The reviewer owns it and has full responsibility for it. He can reject it for any reason without even stating a reason -- correctness, readability, indentation, uselessness. He can also accept it without reading it. My point is that if the coder and reviewer are working off different definitions, the review is likely to be unpleasant for both and I could see how people wouldn't volunteer to review code.
|
|||||||||||