I don't really buy the two main criticisms around code review. They're stated as intractable downsides but are really just cultural issues around how you perform code reviews.<p>If your coworkers are sending changes for review that don't explain why the changes are being made (and therefore provide the context), reject it until they write a proper summary/test plan that does explain it. You should review the "metadata" of changes before you ever review the code itself and if done well, you'll probably be able to guess what a lot of the incoming changes are before reviewing the code. This makes it easier to spot code that deviates from the stated purpose of the changes and can help identify faults in both understanding of the overall problem or the system being built, especially in more junior engineers.<p>If your coworkers are taking to long to review changes, work with your team to incentivize expedient code reviews, or talk to the people that aren't reviewing enough code to help everyone else get more done. You should also utilize a stack that lets you continue progressing with your work even when waiting for reviews (like stacked diffs).
The issue with code reviews is that most of them should be prototype reviews before you are even half way done, or design reviews before you even start. Or just dialog around some code or problem, along the way. Looking for quality issues or obvious mistakes after the fact is very rarely useful. I still think it's a good idea to have them as a sign off (e.g. so you at least have 2 people <i>knowing</i> what's happened), but other than that I don't think they provide that much value, at least not for people who are many years into a project and code base.<p>But still we do these after-the-fact code reviews because I don't want to bother my colleague N times in the process. So I churn away until I'm done. And then not to bother them twice, I also polish it. And THEN I ask for a review. The reviewer sees something polished and finished, and assumes that a) since it's polished I must have spent a lot of thought around whether it's the right thing, and obviously anything they can do in the review time which is never going to exceed 2% of the dev time, is rarely going to result in them second guessing that and b) since it's polished they know they'll probably step on some toes if they say "throw it out and do something completely different". They can see the effort. The relationship with a colleague is more important than even the health of the project. So they approve it.<p>The problem of course then is: how do you get from the "too late code reviews that they are rubber stamps" into a process of dialog, half-way reviews and iteration? Other than pair/mob programming I don't know. We say we don't want more processes, but whenever there isn't a process for this iterative behavior, the result is people developing for too long on their own and producing results that are too far gone to be shot down.
This article has a "The Issues with Async Reviews" section but not a "The Issues With Pair Programming and Mobbing" one, so it's not a particularly considered piece.<p>I have found that pair programming destroys the ability to coherently think about a large portion of work for a large portion of developers. It is not easy to be "on" all the time, and a lot of people work at a pace or in a structure that is not compatible with that.
An intriguing article, although my opinion on async reviews is much more positive. :-)<p>There are many arguments for both sides.<p>Pro pair / mob programming:<p>* Faster feedback.
* Less time spent on solutions that might be rejected.
* Better knowledge sharing (for those who participate).<p>Pro async review:<p>* A more neutral, "external" view. (Smaller chance of groupthink.)
* More empathy with future readers of the code.
* Encourages to make assumptions and decisions explicit. => Better knowledge sharing for those, who weren't involved in the writing of the code.
We don’t do code reviews anymore. We have a good quality gate (e2e tests) and pair programming. All development happens on main and gets directly deployed to prod when the tests succeed …<p>I hope I never have to use git flow or alike again in the future!
(I am biased: I am one of the founders of <a href="https://graphite.dev" rel="nofollow">https://graphite.dev</a> )<p>Flip side of this, you can tackle both of their issues with better tooling and not throw the baby out with the bathwater.<p>Time lag: Stacking (<a href="https://graphite.dev/stacking" rel="nofollow">https://graphite.dev/stacking</a>) is a powerful technique that allows developers to keep developing even while others read through their code.<p>Context gap: you encounter the same issue in an IDE when exploring a new part of a codebase, and there are tools to help you there (click to see function def, open relevant tests, AI summarize what its doing - we've been trialing the last one internally and it's so cool). Your CR tool should offer all of that, and at many large companies (Google, FB, MSFT, even Jane street) it does. GitHub is just deficient here.<p>I agree with the other comments in this thread that the main problem is that what should have been an architecture / prototype / etc review happens in CR, and I agree with that. But I think moving away from CR worsens that problem not improves on it.
Am I the only one who thinks it would be incredibly rude to ask for a code review before I'd actually fleshed out my ideas and also cleaned up to make it presentable? For that matter, if early code review is for design decisions, doesn't that mean tasks are large enough to require plans and reviews of those?<p>Shifting code reviews left seems to be either (or both of):<p>- look, I made a poopie!<p>- let's do Big Planning Up Front (agile has failed)
This reads far too basic, like someone read some other blogs and tried to write one without out blatant plagiarism. It's just code advice cliches.
My biggest issue with code review is that people rely on it and trust it too much. It does certain things in the "better then nothing I guess" way, but gets hyped as if it was all there is to teamwork.<p>Yes, it does little bit to spread knowledge - but it is not much effective at that. Actual occasional architecture session where ideas are explained is much better. Yes, it does little bit to catch bugs, but having actual testing in place does much more. Yes, it does something to teach junior, but for Christ sake, currently it leads to "give them task with no guidance, let them figure it out and then tell them about everything they have done wrong". That is just about the worst way of teaching people.<p>And it creates social issues we don't want to acknowledge, because code review is sacred and cant be criticized. Which leads to kindergarten level of advice to people who run into issues like "dont take it personaly, be nice". Which is next to useless if you have an actual social issue going on in your team.
I think most issues brought up here would be addressed by writing rfcs and have those go through a review cycle, potentially with attached code that is clearly understood to be a proof of concept. It leverages async by giving someone the autonomy to do clear thinking and clear coding, while avoiding pushing the feedback cycle to when all the work has been done and everybody is stuck with a “lgtm can you reindent the last line” review of a fait accompli .<p>Added benefits are proper top level documentation, and a log of the discussions and compromises made to cut short on future design discussions.<p>Edit: typos on mobile
> a traditional async code review process<p>This is not a very old "tradition." We started code reviews back in the 2000s with in-person small group code reviews, and only later moved to the tool-based async code reviews that are ubiquitous today. We adopted good ideas from a web article "Effective Code Reviews Without the Pain" ( <a href="https://www.developer.com/guides/effective-code-reviews-without-the-pain/" rel="nofollow">https://www.developer.com/guides/effective-code-reviews-with...</a> ). We had folks enter comments in advance but went through them as a group, using social customs like starting questions with "did you consider..." to keep it cordial and non-confrontational. A lot of these reviews generated good discussions about code quality, clarity, etc. We didn't review 100% of code using this process, and we didn't always require the review to be completed before merging - the main goal was more to improve all developers' habits than to improve one particular chunk of code.<p>Moving to the current tool-based async code reviews has pros and cons, but now it takes effort to ensure they stay positive, and the async process eliminated some of the great team discussions that the old-style reviews had.<p>Perhaps the ideal solution is a mix of both approaches?
I am biased here since I am the creator of an async code review tool (codeapprove.com) but I think that many teams don't take the time to invest in their code review process and tools enough. For something that most of us do every day (often multiple times per day) there's surprisingly little focus on how to do it well.<p>When I joined my last team they had a bad code review process. Basically once a day or so the CTO and one or two other senior people would look at all the ready-to-merge PRs and give them a thumbs up or thumbs down. I pushed for a much more rigorous but inclusive process. Everyone would do code reviews, every PR would have two approvers, we'd talk about how to do review well and we'd invest in some more automations to solve common pain points. Not only did we get faster as a team (contrary to popular belief, more reviews != slower) but we also got much more thorough feedback AND we were able to consciously propagate the right patterns throughout our codebase in an organic way.<p>It's no exaggeration to say that good code review turned leveled the team more than any other engineering process change we made. More than testing or sprint improvements, more than our management changes, etc. It's highly underrated!
<i>Decide if the right structure was chosen: Even if the code is doing the right thing, its structure may be ineffective or inefficient.</i><p>I can't help thinking that if you're waiting until code review to determine if someone on the team wrote the code in a way that will pass a review or not then you have some serious comms issues on your team. If this is a problem then you should be doing mini reviews during the dev process as well as a code review at the end.
This is the code review model I've arrived at after going through a number of different processes throughout my career:<p>Code review is a health check from your teammates who don't have the authorship bias. It is not a replacement for testing, both manual and automated. Testing is solely the responsibility of the author, code review is just a design lint check and/or knowledge sharing opportunity. You can still have a QA team of course, but that is usually decoupled from application development and is done post-merge anyway.<p>Every shop is different so YMMV, this is just a personal take.
There are multiple good points.<p>Which I would addressed first by tooling of course like linters and automated code checks so low hanging fruit is out of the way.<p>Second thing of shifting left is what I call “developers alignment” - which is yes a meeting where people regularly discuss! things instead of coming up with stuff ad-hoc during review or expecting others to get the context from thin air. Then repeating “obvious” rules regularly because people forget and not expecting people to remember things by heart.
pair programming is not a replacement for code reviews; the two people who worked on the code will have the same blind spots (even if the quality of the code is likely going to be better) and a fresh pair of eyes will offer the same benefits as in the regular case