No, every project I have worked on has review gated commits. It is a /basic/ step in ensuring that a project maintains a high quality codebase. Review does cause delays, because reviewing takes time, but we've generally found that the speed "gained" through poor change control is more than made up for through bad code.
Review also shouldn't be causing ego antagonisms. You're coworkers. You have to be able to work together. That's called having a job.
Every project I have worked on in a professional setting has had mandatory code review, with review acting as a gate. That's been required in order to maintain software quality despite the quality of engineers I've worked with.
Agreed. I'd like to add that PRs are not only a means to review code to improve that code. They're also way for new hires and jr devs to learn new things, how the hive mind thinks, etc. This is, ideally, today's PRs help improve tomorrow's code as well.
You can have perfectly fine, high quality, peer reviewed code without PRs.
Only if some regulation requires sign offs from e.g.other depts. PRs are inevitable. In all other situations they are at best an inneficient workflow and at worst a Kafkaesque circus.
Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.
This is not an opinion. It's a scientifically proven fact. As laid out in the book Accellarate.
> Peer programming, daily checkups, a rock solid CI, and, above all, trust in the professionalism of your team are some ingredients for high quality, high throughput software development.
Absolutely agree, the collaboration should happen sooner in the process, and I would add that the team should probably also have made at least a high level proposed solution together before the work even started.
It's alright, when I started there I turned those 21 regular expressions into a single enormous regular expression that could only be understood at the time of writing, and never understood by mere humans again.
> Review also shouldn't be causing ego antagonisms.
Yeah but the tool itself is antagonistic, because it imposes a workflow of open source, a workflow that also is antagonistic with clear and absolute power. So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power. It's not hard to see how that becomes somewhat complicated.
Anyone who can't handle having their code reviewed before it goes into production is not welcome on my team. You can call that antagonistic or not, but I have always found that the best engineers are the ones who are able to separate criticism of their code from criticism of themselves. Get that type of people on your team, and code review is no longer antagonistic: it's just an egoless feedback process that improves the end product, which is what all members of an engineering team should be working toward.
I'm not criticising the review itself as much as rather the PR workflow and the tools of github, especially the blocking mechanism. It's my opinion that collaboration should happen much sooner rather than being pushed to the end with the review of a PR, and that you should have designated people who have the power to sign something off as production ready.
If a team inside a company want to gather inspiration from the open source world, they should look at how the owner teams of open source projects work internally, not how they work together with outside contributors.
And, it's a very common "solution" in tech to just simply not have any feelings, but people have feeling and that you can't turn off, and that's a very important contributor to work satisfaction.
There is nothing about github hat prevents you from working that way. I don't see what your issue is. But, I will say that I don't want people who can't separate criticism of their code from criticism of themselves on my team. You can certainly have whatever feelings you want, as long as it doesn't get in the way of producing the best possible product, all things considered.
Yeah there's nothing that prevents you from using a tool in exactly the opposite way that it's designed to be used, but it's also pretty unlikely that it's going to happen or that it's going to be successful.
Just make a PR early and discuss the code as you build and make changes, not sure in what way the tool wasn't made to do that. Then you also get the discussion interleaved with changes, resulting in basically perfect documentation of how the code was made and why.
The only thing PR's lack in that regard is that if the reviewer accepts it then it gets added automatically, while ideally you should re review things after the reviewer has accepted it. That way they wont accidentally accept prototype changes.
I'm beginning to think you've never actually used github. Are you trolling?
The tool does not stop you from working in the way you suggest. Maybe it's not what the engineers who wrote it originally envisioned, but it's both simple and easy to work in the way you suggest.
If I want early feedback on something I create a draft PR and ask for comments. I can then let people consider my approach asynchronously, in their own time.
> I don't want people who can't separate criticism of their code from criticism of themselves
Yeah good luck with that, nobody can completely separate criticism of their work from criticism of themselves. You are making your job as the team leader way to easy for yourself, "I only hire robots, that's how I solve all these pesky people issues".
In over a decade of enforced code review experience, I've had one developer who was too immature to take feedback. Some folks take it personally and they shouldn't as long as the feedback is about the code. This requires some work on both reviewer and reviewee.
The guy who couldn't take feedback (person A) had code merged in that wasn't properly tested. Person B said, "hey A. I could use some help. We wrote some tests around $feature that were missing and the tests show the feature doesn't work. We see $unexpected results. Wanna make sure we agree on how this should work."
Person A: no, it works, I tested it.
B: could you help us identify the flaw in the tests then - they seem rock solid.
A: no, my code works.
B: ... ok, can you join us and talk through it?
A: no, it works.
A was removed from the team after management came in and A continued to not acknowledge his code could be wrong.
This was aberrational. We, as an org and as a team, constantly strove to keep the focus on the quality of the code. And, yes, his code was borked.
In this example, of course person A is completely in the wrong, but this is a bigger problem of being so immature that you can't admit any fault.
My suggestion is more along the lines of: use a pairing session for review so that you can bring your empathy as well as your technical expertise, and make it a step in the process just like any other steps (testing, PO approval) etc, and just trust people to do it.
I don't think there's any reason to use a tool from open source, to make code review enforced and with passive aggressive online communication and "blocking". Just seems to make work more painful, and less efficient as well actually.
Oh wow, kudos to you, not just for having to deal with that, and getting A removed, but for being so diplomatic about approaching him on the issue.
And yes, someone that lost is pretty rare, but I'd say lower-grade versions of non-transparency and making their work hard to follow is pretty typical (and frustrating).
and yet many people on this subthread alone have worked in review-required jobs, and have not had a problem with it. They've also provided reviews for other people's patches, and presumably were also able to do it without personal attacks.
It's the bare minimum of professionalism.
If you are unable to separate feedback on your work, from attacks on your person, you are lacking some fairly fundamental skills needed for professional engineering.
>So using that tool suddenly brings that antagonism and power into a team which is supposed to be 100% collaborative, and it also only does it periodically and with random and different people in power.
Is code review not collaborative in your experience? In every team I've been on/run we've split code review comments between suggestions/questions and actual "I will not let this go into prod if my name is attached to it" blockers. Code review has been 99% the first set of comments and only rarely do I see anyone actually block reviews over things like style and what not that have been mentioned here.
I can't even imagine those topics getting into code review as a blocker as if we have actually strongly held opinions on mechanical issues like that, they are integrated into the various linters used so you don't even need human eyes to catch the issue.
I've had patch review as back and forth comments in email, in bugzilla, in myriad other bug databases.
If you can't send out an email with your patch as an attachment, and get feedback, then we have a problem, and the problem is not the adversarial nature of review.
But why would you use such a remote asynchronous late stage feedback loop, if you are literally sitting in the same room as your collaborators, during the whole development process?
> if you are literally sitting in the same room as your collaborators
Your very premise is wrong. At any sufficiently large company, you are unlikely to be sitting in the same room as every collaborator and stakeholder, or even proxies for them.
As a simple example, the team I currently work on (on one project of several) is 10 people across 8 US cities in all four US mainland timezones, and the stakeholders and collaborators are across Australia, Asia, Europe and the Americas. A good majority of what I do is pair programmed, yet the pull request workflow is essential to letting _others_ know what is happening and why, and to allow them to have asynchronous input into the process.
You might argue it would be better for the team to be in a room somewhere. Maybe so, but the people this project demands live where they live and could not even agree a common location for that room to be if they wanted to. And it still wouldn’t help the other projects…
I was more offended by the assumption that you can simply interrupt whatever work your coworkers are doing, just because you're already in a much less pleasant open plan office.
The goal in review is to try to catch any oversights or errors in the code you wrote. The code you wrote may have been the result of of discussions with your coworkers, but the code still gets reviewed, even if by people you discussed the implementation with. That review will occasionally find things you missed, and then you can cycle, and re-review.
By definition, the review can only happen at the end of change development.
Review also shouldn't be causing ego antagonisms. You're coworkers. You have to be able to work together. That's called having a job.
Every project I have worked on in a professional setting has had mandatory code review, with review acting as a gate. That's been required in order to maintain software quality despite the quality of engineers I've worked with.