Thursday, February 20, 2014

Code Reviews and Bad Habbits

Quoting particular sections:

There are two fundamental problems with single-patch review systems:
  1. They encourage lumping at-best-weakly-related changes together. Frequently, when I start implementing a new feature, there are at least three steps: first, refactor the existing code to make it clean to add the new feature; next, add the new feature; and finally, add unit tests. The bigger the feature, the more likely each of these steps is likely to itself consist of several logical steps. If you can store several distinct commits in a single review, then you can simply keep these commits grouped together. But if I’m in a single-patch system, I’m going to be strongly encouraged to do everything in one massive commit. That’s especially frustrating because refactoring existing code, and adding new code, get lumped together, demanding much more mental energy on my part to figure out what’s actually going on in a given review.
    You might argue that you can keep the patches split, creating one review per commit, but that’s actually worse. At best, you’re now separating the tests from the feature, and the refactoring from the motivation for the refactoring. But the real issue is that many single-patch systems make it very easy to approve any one of the commits in isolation, which is exactly the opposite of what you want. Thus, one-review-per-commit switches the balance from “annoying” to “dangerous.” Not really an improvement.
  2. They encourage you to hide your history. The whole point of a source control system is to tell you the history of how code got to be the way it was. I want to be able to see what it looked like yesterday, and last February at 2 PM, and anything in between. Sometimes that’s because I know that the code worked then and doesn’t now, and I want to know why, but lots of the time, it’s because I want to know why a given change was done. What was the context? What was the motivation? When you just keep updating one single patch the whole time it’s under review, I’m losing tons of history: all I’ll get is a single finished product as a single patch, without any of the understanding of how it got that way.

http://bitquabit.com/post/code-reviews-and-bad-habits/

No comments:

Post a Comment