I despise code reviews, because the criticism is shallow, and offered too late, after the work has been done. I want the review in real time, and at a greater depth of insight. This is what pairing provides.
At my previous job reviews were required before you were allowed to check in, meaning if it wasn't good enough you had to rework it. This meant junior engineers were quickly brought up to speed with code convention and best practices. If you didn't adhere to them your code wasn't approved.
I also found it a great way to catch subtle bugs or difficult to follow logic. But it does require actually having and taking the time to carefully review things, rather than giving them a passing glance.
As I alluded to earlier, I've never encountered these sorts of problems in the healthy companies I've worked at (but I HAVE encountered them at unhealthy companies).
But if that fails, it is definitely better to review and catch it after the code is written than to submit badly written code out of fear of wasted effort. In my experience the vast majority of tasks are easily addressed by a single person and most proficient programmers will usually produce perfectly adequate code which doesn't raise any concerns. Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task) but saves a lot more time and effort in avoided bugs and issues than the cost of having to rewrite something every now and then.
Additionally, any single submit should be kept to a manageable size so that they're easier to review and issues are caught early. The only times I can remember having insisted someone rewrite something non-trivial have been when they misunderstood some fundamental issue, did not communicate progress as they should have, and did not follow guidelines of solving a single task per submit. That type of behaviour is in itself a pattern which needs to be addressed.
Yes, without a doubt.
> Having reviews to catch the odd lapse in judgement or logical misstep doesn't usually add a lot of overhead (as opposed to allocating two programmers to every menial task)
Having paired for years, and soloed for years, I have found that the downsides of allocation are overridden by the benefits of pairing, one of which is real-time code review.
In some cases it's invaluable, absolutely. I just don't think that's a large percentage of the code written on a daily basis. For most tasks pairing seems to be either
1) one person tapping out simple code which practically writes itself while the other lazily nods along.
2) One person solving something tricky which happens to be their area of expertise while the other stares blankly. Maybe at that point it would be useful to explain things in detail, but that can make development take much longer and if something like this requires rethinking halfway it can be quite taxing. In that case, I would prefer waiting until it's done and compiling the result as an annotated review. Which can be a great forum for asking questions.
I've had great success with breaking larger changes into smaller PRs that get code-review-merged into an integration branch. Then the integration branch gets merged to your main branch once everything's done and verified.
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.
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.
It seems like there is something about pairing though that guarantees a focus on shallow insignificance. I don't like dismissing something that has smart proponents but I just can't find a way to make pair programming anything other than negative. The real time aspect rules out depth.