3 ms·

Code reviews sometimes seem like a micro-management tool: they often boil down to quibbling over things that are quite superficial, while simultaneously ignorin

by rmk 5y ago

Code reviews sometimes seem like a micro-management tool: they often boil down to quibbling over things that are quite superficial, while simultaneously ignoring or missing big problems that lurk under the surface. Without some sort of automated linting and code style enforcement, this is what happens in code reviews. They serve as a reminder to the developer that his work is not trusted, and someone else must keep an eye on him at all times. This is very bothersome to people who have been around the block a few times, and are already capable of producing robust, clean product (this is often a pitfall of having someone who's just out of college reviewing code from an experienced dev). Another problem I've observed in code reviews is that they turn into impromptu redesigning sessions. Most devs are quite opinionated, and code reviews often become an avenue to engage in bikeshedding that can actually harm morale, not to mention velocity and product quality. The trouble with pairing is that it doesn't remedy these problems effectively, while simultaneously reducing the number of hands doing work. If two people just don't get along, pairing them is not going to magically solve the problem. And I guarantee that there will always be people who simply do not see eye to eye on many important things in most sizable teams.

randomswede 5y ago
Yeah, code reviews (like any other process) can be done wrong. The way to propose a change, in a "good way" (I am assuming here that no one has unilateral capability of merging to master, and things thus have to pass through review), to my mind, involve doing a few things. <ul> <li> keep the change small ("small" here is a very fungible quality, but "probably less than n * 1000 lines") <li> does NOT mix "fix bug" / "add feature" / "refactor" (arguably, there are cases where feature-adding and bug fixing are instrinsically linked, let those through) <li> has a change description detailing the why of the change (the "what" is the change itself) </ul> As a reviewer, your task is not to critique the design (that is why everyone agreed on a design doc up front, no?). It is OK to review the structure of the code, though. Hopefully, you have a "run this through a linter / style-checker" integrated into your pipeline, so that should be pretty much already done. But, if there are a few things left over, it is OK to point those out. It is OK to point out that things that should be (but aren't) tested should have tests. If you can spot an edge case that isn't (or doesn't seem to be) tested, ask for that to be tested, also. People need to be permitted to take the time to do a review. With these things in place, a review should be pretty quick (at the pace of up to a few lines per second), and most of the time frictionless. But, it does require some discipline, both from the writer and the reviewer.

by rmk 5y ago

randomswede 5y ago

Yeah, code reviews (like any other process) can be done wrong. The way to propose a change, in a "good way" (I am assuming here that no one has unilateral capability of merging to master, and things thus have to pass through review), to my mind, involve doing a few things. <ul> <li> keep the change small ("small" here is a very fungible quality, but "probably less than n * 1000 lines") <li> does NOT mix "fix bug" / "add feature" / "refactor" (arguably, there are cases where feature-adding and bug fixing are instrinsically linked, let those through) <li> has a change description detailing the why of the change (the "what" is the change itself) </ul> As a reviewer, your task is not to critique the design (that is why everyone agreed on a design doc up front, no?). It is OK to review the structure of the code, though. Hopefully, you have a "run this through a linter / style-checker" integrated into your pipeline, so that should be pretty much already done. But, if there are a few things left over, it is OK to point those out. It is OK to point out that things that should be (but aren't) tested should have tests. If you can spot an edge case that isn't (or doesn't seem to be) tested, ask for that to be tested, also. People need to be permitted to take the time to do a review. With these things in place, a review should be pretty quick (at the pace of up to a few lines per second), and most of the time frictionless. But, it does require some discipline, both from the writer and the reviewer.