If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth. Instead a cursory glance seems acceptable, because, after all, other people are looking at it.
> If you have three reviewers, no one feels personally responsible for checking it line by line and thinking about the code in depth.
in my experience, this is already happening. When you get pinged to review PR after PR and have your own work to do, it's easy to skim. Requiring 2 reviewers operationally has often meant i'd get a chance to look and take a deep dive after someone caught the glaring issues.
When it's ok for just one dev to sign-off, time and time again that's how i've seen bugs/debt sneak in. I think giving organizations this option is only a plus, and is not really something we need to have a public debate about; it's an organizational decision to use or not use this that github has facilitated.
Two reviewers seems optimal if you can afford it. Possibly with one person doing the bulk of the reviewing and the other more in the role of providing oversight and breaking ties if they occur.
I use a coaching approach for new hires at my startups.
It can be good to do this outside of the public eye, so the initial cycle of "wtf, why is this considered standard" followed by "well, it's not so much that this is the best way, but it's more likely that we'll be able to understand each other's code if we all do it one way" can be done in private.
Once the engineer is well-versed in the system, they are unleashed to quietly subvert it, if that is their will :-)
It’s important to be consistent with your online voice though. If you’re always blunt and terse then, generally, people won’t take it badly. But if you slide between extraneous gestures and out right profanity then eventually someone will take it personally.
I don't want to enter the people vs. systems debate but certainly a good system can help.
1. You've been working on some non-trivial change for long enough that you find some element of it trivial and not worth documenting, but others have no idea why it's there.
2. You've been looking at something so long that a "temporary code, I'll fix it later" starts looking natural and it takes someone else to call it out.
I'll set aside the fallacious premise that everybody can stack their team with 10x engineers. Some of us actually work with interns and juniors.
That being said, your comment doesn't invalidate the need for a better review system. Software quality isn't defined by any one tool or process; the product is the end result of a chain of tools and processes...
And I've coded with (and hired) interns and juniors, so it's not just a selection effect. In fact, the worst code I've seen came from a senior engineer, with strong opinions, who wrote the world's most blecherous code. We fired him, but not soon enough.
What I’ve decided is that it’s folly to try to enforce accountability. A good system provides visibility, that’s it. Who did what? Document it, but allow implicit social process to handle enforcement. Hard rules just lead to people cooking the books, or opting out of the system entirely.
I feel the same about code review. Document who reviewed. But it’s up to individuals to take responsibility for the outcome. And at the end of the day it’s the committer’s code. It’s their name in the git history.
Companies have managers. It's very important that mangers at a company set the tone for using mistakes to create better engineering processes. And that involves a lot more than shaming the committer.
I developed an app (https://pullreminders.com) that helps teams stay on top of open pull requests and in talking to customers, there is a huge demand for features and tools to support a better code review process.
(Disclaimer: I am the creator of Pull Reminders)
In my experience, bugs pop up more often in changes where reviewers aren't honest in how well they actually understand the changes, and are maybe afraid of admitting their confusion (even though it's usually the case that if one person in the room is confused, everyone is).
This change allows you to prevent that from happening, by requiring as many reviewers to approve a request as your team has—so that everyone has to approve it. Previously, GitHub only let you set one required reviewer, which enabled diffusion of responsibility.
The responsibility (or blame?) for bugs is diffused over more people, so people will care less.
I'm not sure that I agree, but I can at least understand the argument.
There is no way I am looking at every single line, and even if I did it doesn't mean I understand the code.
Having two people check something over is a good thing, telling two people that they should both start simultaneously to do work that might be duplicate is bad.