1. 5

    I found this article hard to understand because my point of view is that instead of trying to make life as-easier-as-possible for the reviewer I actually would prefer to make life as easy as possible for the person who is actually doing the work…the author.

    One of the fundamental asymmetries of a code review process is that the reviewer spends a small amount of time but each remark or request they make is a large demand on the time of the author. It’s very cheap to say “why not do it like X?” where X is a fairly superficial change but will take a substantial amount of time. The burden of proof tends to fall on the author who often ends up doing the work even when the benefit of X is extremely marginal just because they can’t easily demonstrate that.

    And this is before getting into the numerous common failure modes of code review processes that I can think of off the top of my head:

    • people who make them adversarial (tremendous harm done to the author by the review)
    • people who ask that each change made to the code put it into a perfect, final state (again, real pain for the author)
    • people who discover new specs for the feature at the review stage (yet again a real pain for the author)
    • workplaces that require >1 signoff (yet again…pain for the author, especially if your referees get into a bunfight about the change)

    I try to fight these behaviors myself when I review code by only considering two angles:

    • is it an improvement?
    • will it cause any operational problems?

    Any other matters are not something to be solved via github PRs, IMO.

    1. 5

      There’s another fundamental asymmetry in the way we interact with code, that is aptly described by the adage “code is read more often than it is written”. I believe the spirit of code review is to optimize code as it is read, rather than as it is written. Responding to feedback in a code review is a one-time effort on the part of the author, but saves the time of many people down the line, many of whom are likely to have the same questions raised by the reviewer who provided the feedback. A good example of this is the point about answering review questions in code comments, rather than in the review itself.

      The failure modes you described are very real, but I think these problems exist in any type of review. Giving feedback is a discipline just like any other.

      1. 2

        It’s not a zero-sum game! …and the “make the other person’s life as-easier-as-possible” attitude should actually be present on both sides… When the other side is not doing that, social, diplomacy and conflict resolving skills start to come into play… first it’s good to catch a call and try to check what’s behind the few curt words in a review; with an assumption of good will, it often shows up a seemingly annoying comment was completely unintended… other times both sides may have an opportunity to learn some good reviewing practices and guidances… finally, sometimes there might just be some conflict and/or toxicity in the work environment, that will show up anyway this way or another, and might need various other solutions (or just leaving the place), but I don’t think that’s in any way different with code review than any other interactions between people at a workplace…

        1. 3

          I don’t think I’m really saying that it is a “zero-sum game” but I am saying that there are tradeoffs to make. It isn’t possible just to merrily make everyone’s life as easier as possible. The truth is that, as humans, everyone finds it very validating to sit in judgement and correct others for the purity of the codebase. That is why so many code review comments are suggestions to golf things together, or split them apart, and so on. I’ve even had to deal with PRs where one round of review has said “golf it together” and then the next has said “no, no, split it apart (but differently)”. I’m sure I’m not the only one.

          I also think you’re not right on toxicity in code reviews necessarily reflecting the work environment. I actually think that for various reasons code review brings out the worst in many of us. There is tremendous pressure on many in organisations to be clever or to be superior (something that official hierarchies can serve to encourage). This can be a minor problem in working life that for whatever reason becomes a major problem on github.

          Github themselves haven’t done many good turns on this. If I were them I would be commissioning research into how their PR features are used and changing the UI to accommodate more positive patterns. But they have barely changed the PR form at least since 2012. It’s a shame.

          1. 2

            That’s a really interesting elaboration, thanks! Much more nuanced than what I thought initially (which would seem to somewhat prove your point…), and quite some food for thought for me. Though in this context, I wonder why I don’t believe I personally really see the level/heat of debate as notably different in code reviews vs. outside at the workplaces where I’ve been employed. I totally do also recall in-person heated and/or passive-aggressive arguments (said mildly) I’ve been involved with occasionally (I can recall some about architecture and infrastructure), and I honestly don’t think the frequency of those in code reviews was notably different. Though obviously not a hard science/measurement, completely anecdotal/subjective.

            (Also, as a side note, I’m once more amazed how the conversations on lobste.rs tend to often blindingly quickly spiral into civility and depth… that’s an awesome community to be a part of! Possibly a good moment to send some much needed love to the Mods too… 💓)