If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
PR review is probably at least a little performative.
But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it.
I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer.
This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.
There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs
You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed.
OK.
There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe.
Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
Agree with the overall sentiment but disagree with
> A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.
Another tip: Use `git log --first-parent` and `git log --merges` to hide the intermediate commits. `--first-parent` also works with `blame` in modern git. These mean you don't have to look at all the small commits when browsing history, only when you want to dive in deeper.
- Keep PR messages short and to the point.
- use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta.
- put the ticket in the branch name. Non negotiable.
- Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you.
- add links to your ticket. Docs, readme, o11y graphs, etc.
- Link ticket in PR for easy access to additional info
- Admit if you don't understand what your looking at. Better to pair and keep moving forward.
- if you request changes, stay available for conversation and approval for the next few hours.
- punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though.
- Avoid nits. This causes a loss in momentum and v rarely is worth changing.
A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base.
- Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.
- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.
- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
A while ago at a past job, I was working on OpenERP (now called Odoo I think). This "community" had instated this kind of "mandatory community review" policy so that each change had to be reviewed by X developers from other organizations. I kind of virtuous web of review.
But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.
Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.
So I write down my estimate in our tracker: 1 month.
He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.
All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.
So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.
So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.
If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this.
If you still need to move fast, then don't.
This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.
What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist.
You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.
Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.
Unless you have a broader context, reviewing 300 line PRs in 5 minutes is going to be surface level at best. Plus that time comes with an expensive context switch so the actual cost is likely more like 20 - 30 minutes. At this point, I think a reasonable question is why not just use AI for shallow reviews like this? This would free up bandwidth in situations where you really want code reviewed by someone else.
It's a very common refrain but I don't really agree with it:
"How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory."
The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in:
- A big queue of PR's for reviewers to review
- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
- You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.
The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.
I love code reviews and blog posts about them, but I vehemently disagree with all of this advice.
>His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.
>While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.
This is ridiculous!
You absolutely can and should review a PR without demanding its "commit story."
Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.
The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.
The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."
When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.
>How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.
If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.
If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.
I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well you can isolate the "concerns" a frontend depends on backend API, etc.
You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.
And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.
Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.
79 comments
[ 4.7 ms ] story [ 80.7 ms ] threadBut I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it.
I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer.
This, "throw the code at the wall for interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.
There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
OK.
There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe.
Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
> A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.
- Keep PR messages short and to the point. - use as many commits as you need, it's all the same branch. Squash if you want, I think it hides valuable meta. - put the ticket in the branch name. Non negotiable. - Update your ticket with progres. Put as much details as you can, as if you were writing to someone who's picking up the task after you. - add links to your ticket. Docs, readme, o11y graphs, etc. - Link ticket in PR for easy access to additional info - Admit if you don't understand what your looking at. Better to pair and keep moving forward. - if you request changes, stay available for conversation and approval for the next few hours. - punt the review if you don't feel like you can legitimately engage with the content right now. Make sure you communicate it though. - Avoid nits. This causes a loss in momentum and v rarely is worth changing.
- Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing clear APIs. The worst damage is that one module might occasionally be a bit marginal, but that can be an acceptable tradeoff in large projects.
- Does every single change need to be thoroughly discussed? Then you may want up-front design discussions, pairing, illustrated design docs, and extremely close reviews (not just of the diffs, but also re-reviewing the entire module with the changes in context). You may even want the PR author to present their code and walk throuh it with one or more people. This can be appropriate for the key system "core" that shapes everything else in the system.
- Is half your code written by an AI that doesn't understand the big picture, that doesn't really understand large-scale maintainability, and that cuts corners and _knowingly_ violates your written policy and best practices? Then honestly you're probably headed for tech debt hell on the express train unless your team is willing to watch the AI like hawks. Even one clueless person allowing the AI to spew subtlety broken code could create a mess that no number of reviewers could easily undo. In which case, uh, maybe keep everything under 5,000 lines and burn it all down regularly, or something?
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at.
A squashed ticket-by-ticket set of merges is enough for me.
But the thing is: this code is terrible and huge chunks of it are a unholy mix and match of code written for very specific purpose for this or that client, with this very weird "falsely generalized" code. I don't know how to call that: you have some very specific code, but you insert useless and probably buggy indirections everywhere so that it can be considered "general". The worst kind of code.
Anyways, I was asked by my boss to do such a review. I look at it and I realize that building a database setup to be able to properly run that code is going to take me weeks because I'm going to have to familiarize myself with tons and tons of modules I don't know about.
So I write down my estimate in our tracker: 1 month.
He comes back to me, alarmed. A whole month? Well yeah, otherwise I can't even run the code.
All you have to do is look at the code! What? No way, that ain't a review. Well, I ask you to do it as such. I'm not writing LGTM there.
So I was never asked to do reviews there again (in fact, I stopped working on OpenERP at all), but I could see "LGTM" popping up from my colleagues. By the way, on OpenERP tracker, all you ever saw in review logs was "LGTM" and minor style suggestions. Nothing else. What a farce.
So yeah, as the article says, there are some "LGTM-inducing" type of PRs, but the core of the problem is developers accepting to do this "LGTM-stamping" in the first place. Without them, there would only be reviewable PRs.
If you still need to move fast, then don't.
This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you need to make big hops, sometimes small ones.
You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force.
Maybe I've only worked at crappy places
"How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory."
The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in:
- A big queue of PR's for reviewers to review
- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)
- You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you
The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.
>His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.
>While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.
This is ridiculous!
You absolutely can and should review a PR without demanding its "commit story."
Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.
The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.
The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."
When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.
>How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory.
5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.
If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.
[0] https://github.com/sasa1977/hamlet/pull/3
[1] https://refactoringenglish.com/chapters/commit-messages/
When I said my top preference for AI usage, by far, would be to eliminate human code reviews, the response was basically, "Oh, not like that."
You end up creating more work for the reviewer, and most people just simply won't do the work of a proper review. You also don't have the advantage of any CI or tests running across the entire set of changes, so you also have separate CI reports to review. All this adds up for more places for bugs to hide or happen. All the same risks are still there, and you've also added a few more points of failure in the splitting process.
And for what? To end up, most likely merging in one PR after the next, for a feature that should just be all logically grouped together, or just squashing and merging the PR's together anyway.
Get your potential code reviewers involved before you even start coding. Keep them abreast of the implementation. When it comes time for a review they should be acquainted with your work.