[jt2190]

I’m not sure why the author ignores the “… that should block a submisson” part.

The abstract of the paper:

> Because of its many uses and benefits, code reviews are a standard part of the modern software engineering workflow. Since they require involvement of people, code reviewing is often the longest part of the code integration activities. Using experience gained at Microsoft and with support of data, we posit (1) that code reviews often do not find functionality issues that should block a code submission; (2) that effective code reviews should be performed by people with specific set of skills; and (3) that the social aspect of code reviews cannot be ignored. We find that we need to be more sophisticated with our guidelines for the code review workflow. We show how our findings from code reviewing practice influence our code review tools at Microsoft. Finally, we assert that, due to its costs, code reviewing practice is a topic deserving to be better understood, systematized and applied to software engineering workflow with more precision than the best practice currently prescribes.

“Code Reviews Do Not Find Bugs: How the Current Code Review Best Practice Slows Us Down”

https://www.microsoft.com/en-us/research/wp-content/uploads/...

[gwd]

The "that should block submission" is always one of the trickiest parts. There's a saying: "Everyone that drives slower than you is an idiot, and everyone that drives faster than you is a maniac." But it is true that going faster increases danger, and there is a speed that appropriately balances benefit against risk; but everyone perceives it differently.

The same is true of "code smell" issues: Everyone who asks you to change things is a pedant who's slowing down the project for pointless aesthetics, and everyone who pushes back against changes you've requested is a cowboy who is going to make the code harder to maintain in the future.

So in the paper, how did they decide whether a non-bug change "should block submission" or not?

[jt2190]

If a comment points out a bug/defect [1], then it should block.

If you think about it, as bugs/defects are removed, the code becomes more correct and thus more stable because it doesn’t need additional changes to remove bugs, so removing bugs reduces the need for future maintenance.

If we block due to future maintenance concerns what we’re really asserting is that the requirements are unstable, and that removing today’s bugs is less valuable overall because requirement changes will remove the line of code with the bug and replace it with a new line of code with a new bug.

I suppose it depends on the code review process at at a given organization whether that’s the appropriate point at which to block code for architecture/design issues. In my experience the code review step is much too far downstream in the development process and much too narrowly focused on a subset of code to be an effective place for design changes that have significant impact on maintenance.

[1] The paper authors reviewed data in Microsoft’s internal code review tool, which is proprietary, so we can’t see what the specific bugs were.