4 ms·

Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manu

Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manuscript. Reviewing every diff means that every single change needs to yield a good novel. But that's an absurd constraint on the creative process - what if you want to introduce a new important character? You check in a new version of the first chapter where the character figures prominently, but the reviewer complains that the new character's plot arc never goes anywhere. The main goal of this diff-based code review seems to be to prevent people from checking in bad code. But of course you should sometimes check in bad code (especially in the early stages of a project)! You might consciously check in some bad code temporarily, intending to upgrade the code at some future point. More importantly, you generally don't know how or if you're actually going to end up using any given piece of code. Spending a lot of time polishing and reviewing code that doesn't eventually go into the project is a classic example of premature optimization (the root of all evil).

stiglitz 7y ago
“You might consciously check in some bad code temporarily, intending to upgrade the code at some future point.“ The problem is how often devs forget about “temporary” code.
ng12 7y ago
Because grokking 20 diffs across 5 files is a lot easier than grokking the entirety of those files and the context on which they live. If you assume the rest of the codebase meets some reasonable standard of quality the diff should be the only important thing. That said, for particularly complicated changes I'll check out the code and read through it locally but diffing is surely optimized for the most common case.
bt848 7y ago
There's no reason you can't incrementally develop your change over time and then send it when it looks right. You don't need review for every edit. At Google you only need review to commit your change to the mainline repository. For long-running projects it's also permitted at that company to check in whatever you'd like into //experimental/users/<your_username>/...
xtian 7y ago
> The main goal of this diff-based code review seems to be to prevent people from checking in bad code. Perhaps some people use code review that way but what is "bad code"? Code that doesn't match your style guide? Style checks are better applied by automated tools. If you mean code that isn't as polished as possible, then the solution is to agree upon expectations with the rest of your team appropriate to the project and/or task. Better goals for code review are knowledge transfer (both ways), design review (ideally on an early WIP branch), and catching mistakes and oversights.
caconym_ 7y ago
I am a software engineer and an amateur writer/novelist. I am the only person looking at my manuscripts before I finish a second draft. There is no risk that my half-finished work is going to introduce cruft into a shared "codebase" and/or "bugs" into "prod". Either they make it into that second draft state and receive feedback as whole, finished things, or they die inside my computer. This is very different from typical software development as part of a team, and your teammates should recognize incremental green-field work and adjust their review criteria accordingly. It's still important to review this sort of code if it goes anywhere near production systems, though, for obvious reasons. Ironically, since I write in Vim and auto-wrap with par, it's harder for me to generate nice diffs than it would be if I just wrote in Google Docs like a good citizen of the future. When it comes to interacting with editors and other collaborators, though, it's super important to collect and review "diffs" of some form. Horror stories abound of unexpected changes making it to print.
wolrah 7y ago
This is why a lot of projects use version control systems that encourage branching. Develop your feature as you wish in its own private branch, occasionally bringing in the latest changes from whatever your mainline branch may be, then only submit it for review when it's complete enough.
enneff 7y ago
Computer programs are nothing like novels. Novels are intended to be read from start to finish, while the parts of a computer program can be appreciated in isolation; in fact, well-written software is comprehensible when read "in the small". Code review encourages people to build software composed from write small, reusable components. That's a huge benefit. As the guidelines say, they goal is not to check in perfect code, but to move the project forward. Sometimes that means adding code with a TODO comment (and an accompanying bug number) to remove a hack or implement something that's missing, and that's fine. The main benefit of code review is that you're sharing and documenting your development process. Your teammates, by reviewing your code, certify that they understand what you're doing and why. Every commit you make should be explicable and justifiable in the context of code review; if not, then why are you making the commit?
sanderjd 7y ago
In that case you should have an artifact to point to that describes the plan at a higher level. So you send a diff with a newly expanded role for some character, the summary of the diff says, "Refactor chapter 1 to prominently feature Character", then the detailed description says, "Character will have an arc that culminates in Conflict in Act 2 and Resolution in Act 3. See {link} for more details on my current thoughts on where Character is heading." Then this new prose in chapter 1 also has to be good when taken on its own, no spelling or grammar errors and a generally nice sound to it. Then I would consider such a diff eminently mergeable. That is to say, a change doesn't have to be perfect on its own, but the direction should be clear, and it should be locally well constructed.
ScottBurson 7y ago
I wouldn't say I "hate" diff-based code review; it can serve a useful purpose; but I do find that unless I'm very familiar with the code being changed, I don't have time to fully understand the impact of the proposed changes. It isn't until later, when I need to modify or extend a piece of code, that I do that level of analysis — and of course I often find problems then that no one else had noticed (often in my own code too). In my last job we didn't have a review requirement for commits; we mostly relied on this form of catching problems after the fact (and, of course, an extensive test suite). For experienced developers I thought it worked fine; it was only some of the junior people of whom I thought we should probably be reviewing their code more regularly. In my current job we do have such a requirement, and I have mixed feelings about it. There are still plenty of times I don't manage to catch mistakes before they get merged in, and the delays getting someone to review my PRs can be frustrating. The system clearly has some value, but it also has costs. I think I would allow most developers, after an initial probation period, to commit without approval, keeping in mind that someone who shows a pattern of committing bad code may lose this privilege.

stiglitz 7y ago

“You might consciously check in some bad code temporarily, intending to upgrade the code at some future point.“ The problem is how often devs forget about “temporary” code.

ng12 7y ago

Because grokking 20 diffs across 5 files is a lot easier than grokking the entirety of those files and the context on which they live. If you assume the rest of the codebase meets some reasonable standard of quality the diff should be the only important thing. That said, for particularly complicated changes I'll check out the code and read through it locally but diffing is surely optimized for the most common case.

bt848 7y ago

There's no reason you can't incrementally develop your change over time and then send it when it looks right. You don't need review for every edit. At Google you only need review to commit your change to the mainline repository. For long-running projects it's also permitted at that company to check in whatever you'd like into //experimental/users/<your_username>/...

xtian 7y ago

> The main goal of this diff-based code review seems to be to prevent people from checking in bad code. Perhaps some people use code review that way but what is "bad code"? Code that doesn't match your style guide? Style checks are better applied by automated tools. If you mean code that isn't as polished as possible, then the solution is to agree upon expectations with the rest of your team appropriate to the project and/or task. Better goals for code review are knowledge transfer (both ways), design review (ideally on an early WIP branch), and catching mistakes and oversights.

caconym_ 7y ago

I am a software engineer and an amateur writer/novelist. I am the only person looking at my manuscripts before I finish a second draft. There is no risk that my half-finished work is going to introduce cruft into a shared "codebase" and/or "bugs" into "prod". Either they make it into that second draft state and receive feedback as whole, finished things, or they die inside my computer. This is very different from typical software development as part of a team, and your teammates should recognize incremental green-field work and adjust their review criteria accordingly. It's still important to review this sort of code if it goes anywhere near production systems, though, for obvious reasons. Ironically, since I write in Vim and auto-wrap with par, it's harder for me to generate nice diffs than it would be if I just wrote in Google Docs like a good citizen of the future. When it comes to interacting with editors and other collaborators, though, it's super important to collect and review "diffs" of some form. Horror stories abound of unexpected changes making it to print.

wolrah 7y ago

This is why a lot of projects use version control systems that encourage branching. Develop your feature as you wish in its own private branch, occasionally bringing in the latest changes from whatever your mainline branch may be, then only submit it for review when it's complete enough.

enneff 7y ago

Computer programs are nothing like novels. Novels are intended to be read from start to finish, while the parts of a computer program can be appreciated in isolation; in fact, well-written software is comprehensible when read "in the small". Code review encourages people to build software composed from write small, reusable components. That's a huge benefit. As the guidelines say, they goal is not to check in perfect code, but to move the project forward. Sometimes that means adding code with a TODO comment (and an accompanying bug number) to remove a hack or implement something that's missing, and that's fine. The main benefit of code review is that you're sharing and documenting your development process. Your teammates, by reviewing your code, certify that they understand what you're doing and why. Every commit you make should be explicable and justifiable in the context of code review; if not, then why are you making the commit?

sanderjd 7y ago

In that case you should have an artifact to point to that describes the plan at a higher level. So you send a diff with a newly expanded role for some character, the summary of the diff says, "Refactor chapter 1 to prominently feature Character", then the detailed description says, "Character will have an arc that culminates in Conflict in Act 2 and Resolution in Act 3. See {link} for more details on my current thoughts on where Character is heading." Then this new prose in chapter 1 also has to be good when taken on its own, no spelling or grammar errors and a generally nice sound to it. Then I would consider such a diff eminently mergeable. That is to say, a change doesn't have to be perfect on its own, but the direction should be clear, and it should be locally well constructed.

ScottBurson 7y ago

I wouldn't say I "hate" diff-based code review; it can serve a useful purpose; but I do find that unless I'm very familiar with the code being changed, I don't have time to fully understand the impact of the proposed changes. It isn't until later, when I need to modify or extend a piece of code, that I do that level of analysis — and of course I often find problems then that no one else had noticed (often in my own code too). In my last job we didn't have a review requirement for commits; we mostly relied on this form of catching problems after the fact (and, of course, an extensive test suite). For experienced developers I thought it worked fine; it was only some of the junior people of whom I thought we should probably be reviewing their code more regularly. In my current job we do have such a requirement, and I have mixed feelings about it. There are still plenty of times I don't manage to catch mistakes before they get merged in, and the delays getting someone to review my PRs can be frustrating. The system clearly has some value, but it also has costs. I think I would allow most developers, after an initial probation period, to commit without approval, keeping in mind that someone who shows a pattern of committing bad code may lose this privilege.