1. 5

  2. 11

    Some of this is really good advice, but I’ve never liked this in particular:

    Annotate the pull request with comments explaining why the decisions made and why each code change is necessary.

    I generally ask people who add comments to their pull requests explaining the “why” behind why the code in the PR is the way it is, to put those where comments where they belong: in the code. I shouldn’t have to do a git blame and then follow the breadcrumb trail all the way back to the original pull request to find an explanation for the business decisions that informed the code – the pull request is not a good forum for documentation that should outlive the current review cycle.

    1. 5

      Completely agreed. The appropriate response to a very large percentage of “why” questions from code reviews is to revise the PR to answer them.

      I have never had a reviewer push back when I reply, “Oh, good question. I’ll add/update a comment to explain that.”

      follow the breadcrumb trail all the way back to the original pull request

      And this is another danger: I have worked on long-lived projects that switched version control systems and hosting services. It was sometimes literally impossible to follow the trail across those boundaries.

      1. 2

        I completely agree. However I think these are two different types of comments. Specifically:

        • Code comments: These explain the Why and How behind a module, function, or expression in the source code. They should live along with the source code as they will be helpful to future readers who find the code hard to understand.
        • Change comments: These explain the Why and How of PR changes themselves. They should be presented to the reviewer along with the changes that are being reviewed as they will be helpful in providing context for particular additions and deletions. A good example of these are comments explaining why a particular module, function or line of code was deleted. In such cases the only possible comments are change comments because subject of the comment is the expression that is has already been deleted from the source code in the PR. Change comments may also be suitable to be included in the commit message in version control.

        I agree there are many change comments that ought to be moved to code comments, but I think change comments are distinct and still serve a useful purpose.

      2. 2

        I have mixed feelings on the “keep all changes under 400 lines total” rule. I don’t like thousand-line diff bombs any more than anyone else and I try not to lob them at others. But it really depends on what’s in all those lines. It’s pretty easy to blow past that number with low-review-effort changes such as automated refactors on a code base of any meaningful size. Tests can also sometimes have a high code-size : review-effort ratio, but a comprehensive set of tests can make a change easier to review, not harder.

        Or the degenerate case: replace 1000 lines of custom code with a small implementation that uses a new third-party library. With a hard-and-fast change size limit, you would have to whittle away the unused code a chunk at a time. That seems clearly ridiculous to me.

        When I’ve worked on projects where people took a hard line on this rule, it never got as bad as my previous paragraph, but it was annoying about as often as it was helpful. You’d frequently see people shatter a semantically atomic change into arbitrarily-bounded pieces each of which was under the limit but was impossible to meaningfully review on anything beyond a simple syntactic level without flipping back and forth between the pieces.

        I kind of follow Einstein’s philosophy on this: PRs should be as small as possible, but no smaller. If there is a good way to decompose a change into a sequence of self-contained smaller changes, I do that. If there isn’t, I don’t try to force the issue.

        One way I try to answer the “Could this be a separate PR?” question is to ask myself whether I could assign each piece of my change to a different reviewer and get useful feedback.

        1. 3

          I understand that this is a hypo, but “replace 1000 lines with another implementation” is 3 distinct things:

          1. Implement replacement
          2. Use replacement everywhere initial implementation was used
          3. Cleanup

          1 is where the meat of the review is necessary, 2 is fairly easy to review, and 3 should mostly just be deleting code and should effectively just be a noop.

          I personally (for Java) like to reduce this even further to 100-150 LoC in a single review. This doesn’t include tests though, which are usually 3-10x the size of the code, which puts this squarely in the 400-1000 total lines :)

          In the end, the best agile way to handle this is to have a rule. Ignore it when you need to. Notice when this ignoring becomes a burden (many comments on a single review, over many rounds). A good number might be 10 comments or going past 3 rounds of review.

          1. 2

            Yes, it’s truly rare that an arbitrary rule can’t be followed when it pertains to code that you as the programmer add, modify and remove. Things can always be broken down into small chunks, and according to the book, smaller chunks will result in better review feedback from your reviewers.

            I didn’t mention it in the blog post, but changes outside of your direct control (say those 20k changes to package-lock.json because you upgraded a single npm package) don’t need to count towards such a limit. Such files are generated automatically and thus safely be ignored most of the time by human reviewers.

            1. 1

              Also, the part that makes those three things work is the tests:

              1. tests only cover the new code
              2. tests only cover the code that uses the new code
              3. tests covering the old code are deleted

              Even better is that if the tests for 2 don’t exist then you get that as a 0. step (cover usage of existing code with characterization tests to ensure things don’t break) If you see any other pattern, then there’s somethings screwy going on…