Eyes Above The Waves

Thanks for posting! I was referred to a few times in passing, so I feel somewhat obliged to comment (defend myself?) :). I couldn't agree more with all the comments about "design review", "have to understand what the patch is trying to do" etc. To that I'd add that I've seen a kind of review paradox: simple, well-documented patches sometimes result in more work for the author than obscurantist, poorly-documented patches. Not because the simple and well-documented patches are more work (though maybe they are a bit initially). Rather because, like understandable vs. obscure presentations, people are more likely to comment on things they understand. That translates to more review requests and more iterations for the author. With obscure patches, I've seen reviewers just stamp things (with style nits) instead of take the time to figure out what's going on. (That might stem from reviewers not wanting to admit they don't understand the patch; I dunno.) And no, I'm not going to cite examples ;). I don't think conscientious engineers would actually try to exploit that paradox, but it's a disincentive against simple and well-documented code. The solution has to be reviewers automatically r-'ing code they don't understand. Codifying that into the "review process" seems like a good idea, so that reviewers can point to a rule instead of having to "admit" they don't understand the patch. As for having to redo patches because the initial approach didn't make the reviewer happy: nothing makes me sicker than having to tell someone to start over with a different approach. In fact, that makes me sympathetic to the author and more likely to r+ code I would otherwise not. That's a loss to everyone. So for folks working on the code full-time, at least, there's just no excuse for going down a wrong path when asking questions could have avoided that! Another complaint floated was large amounts of code being adding to module X by someone not on the "X team". Members of a hypothetical "X team" should ask themselves why an "outsider" would need, or indeed whether they would even want, to write that code in the first place. And also whether it's better to learn outsider code ex post facto, after it's been field tested and debugged for a long time but maybe not 100% to the X team's taste, or delay the feature/product until the X team has time to write and debug it themselves. There's definitely a point where one or the other becomes preferable, but I can't really say where that point is.

On the subject of reviews, here are two techniques that have worked for me in the past but weren't mentioned in the video: "Starter patch": the reviewer / module owner writes an initial patch with the interface / headers / impl skeleton / etc. they want to see. Then the author bases the 90% "real work" on that. That 10% design work for a nontrivial patch disproportionately consumes the majority of review time (for me anyway), and design can be pretty open ended. So what better way to get a fast r+ than to let your reviewer stamp their own code ;). And if the initial idea doesn't work out, the delta for the revised patch can be communicated efficiently to the reviewer, because they already have the initial model in their head. "Divine intervention": if prose review comments get considerably more complex or ambiguous than changing the code would be, download and edit the patch and use the edited patch as the review comment.