Read on Twitter
One of the more interesting reasons I've seen for teams using Pull Requests is so that other developers on the team can check if you built the right thing. That's wrong on so many levels.
Let's enumerate some of the other justifications.
1. Code quality
I look at a *lot* of code, and - qualitatively - I've not noticed any correlation between the use of PRs and maintainability. The strongest correlations I've seen in that respect are w/ TDD & pair programming
1. Code quality
I look at a *lot* of code, and - qualitatively - I've not noticed any correlation between the use of PRs and maintainability. The strongest correlations I've seen in that respect are w/ TDD & pair programming
...but I suspect those are actually correlations with code quality awareness (the number of devs who think a 50-line method is fine!) and refactoring skills
2. Awareness of the code base
This is one of the most spurious, I think. Like other people's code is only visible in PRs. You can look at the code any time you like, at your convenience. You don't need to be pulled away from what you were working on to do that.
This is one of the most spurious, I think. Like other people's code is only visible in PRs. You can look at the code any time you like, at your convenience. You don't need to be pulled away from what you were working on to do that.
I encourage people, when they pull changes from the main branch, to look at what's been changed. Like a good actor, don't just learn your lines.
3. Learning opportunities
Again with the "If we don't do PRs, how can we learn from each other?"
* sigh *
Again with the "If we don't do PRs, how can we learn from each other?"
* sigh *
4. Paper trail/compliance
This one may be more understandable, if you have to produce documentary proof that code was reviewed. It's a box ticking exercise, sure, and says nothing meaningful about actual code quality. But some businesses *have* to do it, regardless...
This one may be more understandable, if you have to produce documentary proof that code was reviewed. It's a box ticking exercise, sure, and says nothing meaningful about actual code quality. But some businesses *have* to do it, regardless...
...But there are other, non-blocking ways to document code reviews. Many languages have tools available for that that can be used without delaying delivery
5. Debugging
Quite a few have - rightly - argued that code inspections are an effective way of finding bugs. But, again, I don't see much of that actually going on in the field. PR code reviews are very rarely anywhere near that structured or rigorous. So I call "BS"...
Quite a few have - rightly - argued that code inspections are an effective way of finding bugs. But, again, I don't see much of that actually going on in the field. PR code reviews are very rarely anywhere near that structured or rigorous. So I call "BS"...
...and, again, I don't see any correlation between use of PRs and reliability. The complexity of the code, and the speed and coverage (in the multi-dimensional sense of the word) of regression testing is still the best predictor IME
Because they block, and can create a bottleneck in delivery, the average PR code review looks a bit like this
...so for most teams, I'm just not buying this justification
6. Untrusted check-ins
This is the only justification for PR code reviews that really makes any sense. Code is being checked in by developers we have no control over. All we can do is defend the code base from bad check-ins.
This is the only justification for PR code reviews that really makes any sense. Code is being checked in by developers we have no control over. All we can do is defend the code base from bad check-ins.
Some folk have asked what I mean by "blocking". PRs an obstacle to making your changes available for release. That whole check-in process is blocking you. The longer and more complex it is, the more it blocks. And if it involves other devs, it blocks them, too.
Multiple devs checking in to the same code base are basically multiple threads accessing shared data. Each check-in is a transaction that must not break the code. Every check-in has the pre-condition that after your changes are merged with the code in the repo, it must work
The longer the process from initial local merge to seeing it build and pass the tests on a CI machine, the longer that tx lasts. The risk is that other devs can check in conflicting changes meanwhile, but you won't see them. The build can be broken by a queue of conflicting PRs.
To reduce this "merge hell", the team needs to submit PRs more frequently, which can greatly increase the overheard of code review. Submit them less often, and the team may end up spending a lot of their time fixing merge conflicts and/or broken builds.
The larger the team, the bigger the bottlenecks.
I see a lot of teams who say "Not a problem for us", and when I observe them for a while, I see the bottlenecks (and the broken code bases). But it feels normal to them. Like a 50-line method is normal to some developers.
I see a lot of teams who say "Not a problem for us", and when I observe them for a while, I see the bottlenecks (and the broken code bases). But it feels normal to them. Like a 50-line method is normal to some developers.
If the goal is to ensure code quality, then here's a tip. The cleanest code bases I've seen have been written by teams who:
1. Are /mostly/ highly skilled
2. Collaborate closely
4. Mentor juniors extensively
5. Invest in automating testing AND code inspections
1. Are /mostly/ highly skilled
2. Collaborate closely
4. Mentor juniors extensively
5. Invest in automating testing AND code inspections
Many of the things a code review might look for can be detected automatically. These teams generally don't write bad code, but they often have static analysis incorporated into their pipeline as a last line of defence.
And Continuous Inspection isn't an off-the-shelf, out-of-the-box experience. You have to invest in it to get it right. It's advanced Code Fu.
