4 ms·
Having multiple reviewers is often bad. A diffusion of responsibility means no responsibility. If you have three reviewers, no one feels personally responsible
by scarmig 9y ago
Having multiple reviewers is often bad. A diffusion of responsibility means no responsibility.
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.
- dunkelheit 9y agoTrue. But having just one reviewer can be bad too: when two people review each other often, they often "collude" and start to rubber-stamp shitty code. Or the other extreme - they can go into an argument over some issue with no way to break the stalemate. 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.
- greglindahl 9y agoIf you have people submitting shitty code for review, you've got bigger problems than needing a better review system.
- bennofs 9y agoWelcome to open source maintainership
- greglindahl 9y agoIf you have new folks who aren't fully socialized to the project yet, having multiple reviewers is much worse than having a couple of people specifically individually coaching new would-be committers. I use a coaching approach for new hires at my startups.
- aurbano 9y agoCould you describe your coaching approach? That sounds interesting, and is often a difficult problem to solve
- greglindahl 9y agoWell, the basic idea for coaching a brand-new employee is that you hired a perfectly smart person for the job, but your company has a strong coding culture with a particular style that the new engineer isn't familiar with. So it's a big part of on-boarding to not only teach this smart engineer the crazy weirdnesses of your system, but also teach them (what they will perceive as) the oddities of your corporate coding style. 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 :-)
- koolba 9y agoMaintaining a popular open source project is a great way to learn to politely say “fuck you this is crap”. 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.
- dunkelheit 9y agoHere are some of the reasons people will submit shitty code: they (even subconsciosly) think it will be somebody else's problem; they are junior; they don't fully understand the system they are changing; they don't feel polishing the code is valued. Many of these problems can be improved with a good review system. I don't want to enter the people vs. systems debate but certainly a good system can help.
- viraptor 9y agoThere are also 2 cases of tunnel vision that I started noticing in reviews. 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.
- table-delete 9y ago> you've got bigger problems than needing a better review system 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...
- wumpus 9y agoI'm all for good review systems. But any review system that labels a large fraction of the submitted code as very deficient indicates that the solution is to act before review. 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.
- scotu 9y agoI think 1 reviewer works better in general, but multiple reviewers could be made to work if the reviews and the reviewers were separate and unaware of the other?
- codemac 9y agoNo, that's just that no one knows who's turn it is. 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.
- seanwilson 9y agoThere's an element of that but imagine one of the other reviewers found e.g. a massive security flaw and you said the code was fine?
- testplzignore 9y agoI agree. I'd like the ability for reviewers to give different levels of approval. A reviewer who is in a rush can give a "looks okay, but I spent less than a minute looking at it" approval. A reviewer who takes their time can give a "I reviewed every line you changed and completely understand everything" approval. Before the change gets merged, X people need to give a quick approval and Y people need to give a line-by-line approval. 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).
- meesterdude 9y agoIf every dev was equally responsible or thorough, we would be fine with just 1 reviewer. But the reality is that devs are different, and can have good/bad days. Operationally, in every mission-critical workflow, usually at least 2 people need to turn the key, fly the airplane, press the button, sign off, or what have you. There is good historic precedent for distributing responsibility and reducing errors. > 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.
- booleandilemma 9y agoMaybe don’t let the reviewers know there are other reviewers? Like a BCC feature.
- greglindahl 9y agoYeah, over the years no one would possibly notice. Especially smart engineers.
- booleandilemma 9y agoIt’s a psychological thing, like hiding the number of upvotes a comment has received on HN. Everyone knows they’re being hidden, but it helps prevent bandwagoning anyway.
- geofft 9y agoYou get a diffusion of responsibility when there are more people who could do a task than are required (i.e., when anyone can decide not to do it). 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.
- joshuamorton 9y agoI believe scarming is making a different point. While you're saying one reviewer from a pool means no one ever actually reviews the code, he's saying that with 2 (or 5) reviewers instead of 1, you're more likely to have the reviewers examine the code in less detail. 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.
- geekjock 9y agoIt all depends on a particular teams workflow. I think it's great to see GitHub continuing to invest in workflow features like this. I developed an app (https://pullreminders.com 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)
- erikpukinskis 9y agoI’ve lived in a lot of collective houses. I’ve seen a lot of chore systems come and go. 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.
- greglindahl 9y agoCollective houses generally have no management other than visibility. 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.
- megablast 9y ago> 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. There is no way I am looking at every single line, and even if I did it doesn't mean I understand the code.