I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at me. I'm not sure if I'm articulating it properly...but that expectation of a small book report-ish paragraph helped me become much better at critically reviewing PRs.
disclaimer: the majority(not all) PR's were in a very very large terraform codebase.
At work, I've been writing a word document to explain every part of a program I've written and how each element interacts with the rest. It's been very helpful to find things that don't make as much sense as I thought they did when I wrote them, and revealing old code that is no longer used anywhere in the application. And I've caught a few places where outdated .txt / .md / inline-doxygen documentation lies, too.
Writing a summary document really forces you to get to the nitty gritty and catch things that a casual review would miss.
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing!
Our CI automatically deploys a "review app" when someone creates a pull request. This simplifies code review, QA and demoing!
It sounds cool that you create on-demand review app. In a small scale it works. In a larger scale development is absolute impossible due to resource constraints to create on-demand environment for review app.
For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.
This is a deeply unpopular opinion, but I find myself aligning with this more every day. I think code reviews are a fundamentally broken process. Design reviews, before any (non-prototype) code even gets written, are vastly more important than code reviews in my opinion.
Upvoted because these are all great points that I agree with but code review is still worth the trouble in many situations. On my team it helps us keep a consistent style and forces more people on the team to stay up to date with changes. Code quality is improved because of our review process. Pair programming would accomplish some of that, but not all team members would have a voice. Further, reviews are asynchronous.
Sure, reviews are a time sink. No one likes them. We are still better off having done them.
> Most style issues should be caught by automated tools
Some style issues can be, others (“do variables, functions, etc. have meaningful, accurate, relevant names?”) can’t be, until the automated tools are backed by AGI, but at that point “code review” and “automated analysis” become...not so different.
I agree. Code reviews are still a nice "sanity check" where I don't have to understand in full detail what happens and I don't criticize style problems and minor issues. However, I check if the change fits into the bigger picture (in terms of architecture), that the intention of the changes is correct and that tests are added if needed.
Hence, a combination of intense pair programming and knowledge sharing sessions in combination with quick & lightweight PR reviews as well as regular refactorings has worked best in my experience.
> - Most functional issues should be caught by tests (written by another person preferably)
> - Most style issues should be caught by automated tools
I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically.
Better yet, I've made a `npm run fix` command that will fix 99% of them instantly.
If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)
> To bring someone up to speed on your chosen style, pair programming is much faster than code reviews
Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.
That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences?
To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together.
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing!
You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?
You should attack it. You should pick it apart and try to break it in as many ways as possible. Of course you should meter your effort depending on how critical the changes are. It amazes me the lackadaisical attitude towards code review. "It will probably be okay" is the worst attitude to have with computer code. It is the most brittle, brutal environment imaginable.
A reviewer in my team is fond of changing some of the code in the PR and running the tests. The expectation is that if the code is changed randomly a test should always fail. This helps catch untested code paths.
It’s somewhat similar to the practice of automated code fuzzing, only manual and scoped to changes in a particular PR.
Just because there are tests doesn't mean the tests are any good. I've only started my quest in TDD and I've discovered it is rather trivial to write tests that don't really mean anything.
I find myself asking "what is this supposed to do?" and "does this actually do what it is supposed to do?". I tend to drift into a design mindset during test writing (more-so than in code review) that highlights data flow and the basic bits of logic. Even then, I find that I can miss test cases rather easily.
Pair programming is sooo much better than PR’s.
I really loathe reviewing pull requests, no matter if you do it absolutely correct, and don’t miss anything, it’s extremely wasteful of the time of both the coder and the reviewer, even if there are no changes that need to be made. There is the context switch for the reviewer, ditto for the coder, waiting for feedback. Ugh.
Just have two people do the work together, and lose the need for constant context switches and wasted time.
> Just have two people do the work together, and lose the need for constant context switches and wasted time.
What kind of PR process do you have where reviewing the PR takes as long as writing the code in the first place? Because that's the only way pair programming is as efficient as code reviewing.
If I'm doing a PR (e.g. for a new dev), and it's becoming a real bloodbath, I stop doing the PR and do a 1:1 pair programming session w/ the dev. This helps save face and keeps the advice private and inculcation.
I'm doing a lot of code reviews and I never run the code myself. PR owner does that, PR validation suite (essentially reduced CI) does that.
"This code works when you run it" is the last thing I care about when I'm reading the code - it's a given, we won't allow change to merge if it wasn't the case. Code reviews are there to catch mistakes in design, copypasta, how the change fits into the product overall.
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing!
I see some people pulling down code to check for "correctness" but I don't think reviewers should be testers. I see the focus being on architectural feedback, sharing of knowledge, etc.
I can see within some specific fields pulling the code to do a UX run through but that isn't all fields of software development that that is even needed (99.9% of the time I've never felt the need) and mostly I see authors posting screenshots or screencasts to help reviewers with this.
> There is a sense that pull requests actually might make it too easy to comment on a line of code. This ease might be leading to the conflicts and hyper-zealous commenting that frustrates many in our industry.
If there are a lot of comments, I usually find its because of either a disconnect between author and reviewer that should have been addressed before authoring the code or there is something that needs to be automated.
> They never even pull down the changes they are reviewing! To be completely transparent, I’m guilty of that myself.
How often is this even feasible though? sure if there's little difference between the PR branch and whatever the last build I've done is, it wont take too long to build but then I have to install our product and get it set up to test. We have a ton of legacy code that isn't easily testable. We do have a CI build but it takes like ~6 hours to build.
My biggest issue with code reviews/PRs is that it runs counter to trunk-based development, of which I am a fan.
I much prefer to go in and check people's small commits to master than look at a huge PR before it gets merged in, but it requires you (and the whole team) to be really diligent and do it on a daily basis, and when it comes to big features, you lose a lot of the context. OTOH, the amount of times I've been presented with a huge PR of an entire feature where the author begins by saying "sorry for the huge PR" and my predictable reply "I'm having trouble getting the whole picture here, but LGTM?" are countless, so I'm not sure that's better either.
Not sure how to reconcile the two models into something great.
As for the style of code review, I ask the team and adjust accordingly. I can be "big picture guy", or I can do that and simultaneously be "Mr. Nitpick" if people so wish (some people actually do, me included).
I’d be curious to see a survey of how different companies do code review or it’s equivalent. I’d like something about the motivations they had for setting up the systems in certain ways and how they changed them over time. At my employer, code review was once done periodically as a big Herculean effort at each “release” (development stopped for code review) involving printing out the code/diffs on physical paper to read it and a senior person reading basically all the critical code. Over time the system became more automated and digital, and the requirements for review were also reduced. But I have no idea how that compares to other places apart from the first thing sounding very silly.
[+] [-] clipradiowallet|4 years ago|reply
disclaimer: the majority(not all) PR's were in a very very large terraform codebase.
[+] [-] LeftHandPath|4 years ago|reply
Writing a summary document really forces you to get to the nitty gritty and catch things that a casual review would miss.
[+] [-] cbsmith|4 years ago|reply
[+] [-] Chernobog|4 years ago|reply
Our CI automatically deploys a "review app" when someone creates a pull request. This simplifies code review, QA and demoing!
[+] [-] debarshri|4 years ago|reply
For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.
[+] [-] miltondts|4 years ago|reply
- Most style issues should be caught by automated tools
- Most functional issues should be caught by tests (written by another person preferably)
- To bring someone up to speed on your chosen style, pair programming is much faster than code reviews
- To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster
Not sure why we have fallen so much in love with the idea.
[+] [-] TheCoelacanth|4 years ago|reply
- Does the code make sense to a human?
- Is this the best way to achieve the objective?
- Is the thing that the code is trying to do even something that should be done?
[+] [-] beebmam|4 years ago|reply
[+] [-] 01100011|4 years ago|reply
Sure, reviews are a time sink. No one likes them. We are still better off having done them.
[+] [-] dragonwriter|4 years ago|reply
Some style issues can be, others (“do variables, functions, etc. have meaningful, accurate, relevant names?”) can’t be, until the automated tools are backed by AGI, but at that point “code review” and “automated analysis” become...not so different.
[+] [-] valenterry|4 years ago|reply
Hence, a combination of intense pair programming and knowledge sharing sessions in combination with quick & lightweight PR reviews as well as regular refactorings has worked best in my experience.
[+] [-] twistedpair|4 years ago|reply
I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically.
Better yet, I've made a `npm run fix` command that will fix 99% of them instantly.
If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)
[+] [-] omegalulw|4 years ago|reply
Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.
[+] [-] cbsmith|4 years ago|reply
To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together.
[+] [-] the_gipsy|4 years ago|reply
Because organisations obsess about risk minimisation.
[+] [-] yjftsjthsd-h|4 years ago|reply
You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?
[+] [-] kmtrowbr|4 years ago|reply
[+] [-] juanca|4 years ago|reply
In that practice, it's better to run it locally! For each UI file / function, try to get it to execute from a user's POV.
[+] [-] loloquwowndueo|4 years ago|reply
It’s somewhat similar to the practice of automated code fuzzing, only manual and scoped to changes in a particular PR.
[+] [-] all2|4 years ago|reply
I find myself asking "what is this supposed to do?" and "does this actually do what it is supposed to do?". I tend to drift into a design mindset during test writing (more-so than in code review) that highlights data flow and the basic bits of logic. Even then, I find that I can miss test cases rather easily.
[+] [-] sfvisser|4 years ago|reply
And don’t tell me your tests tell you, because I guarantee you they don’t!
[+] [-] elwell|4 years ago|reply
[+] [-] watwut|4 years ago|reply
[+] [-] uvesten|4 years ago|reply
[+] [-] stavros|4 years ago|reply
What kind of PR process do you have where reviewing the PR takes as long as writing the code in the first place? Because that's the only way pair programming is as efficient as code reviewing.
[+] [-] twistedpair|4 years ago|reply
If I'm doing a PR (e.g. for a new dev), and it's becoming a real bloodbath, I stop doing the PR and do a 1:1 pair programming session w/ the dev. This helps save face and keeps the advice private and inculcation.
Don't let PR's turn into public floggings.
[+] [-] dave_sid|4 years ago|reply
[+] [-] the_gipsy|4 years ago|reply
[+] [-] deletionist|4 years ago|reply
[+] [-] avmich|4 years ago|reply
[+] [-] jimbobimbo|4 years ago|reply
"This code works when you run it" is the last thing I care about when I'm reading the code - it's a given, we won't allow change to merge if it wasn't the case. Code reviews are there to catch mistakes in design, copypasta, how the change fits into the product overall.
[+] [-] epage|4 years ago|reply
I see some people pulling down code to check for "correctness" but I don't think reviewers should be testers. I see the focus being on architectural feedback, sharing of knowledge, etc.
I can see within some specific fields pulling the code to do a UX run through but that isn't all fields of software development that that is even needed (99.9% of the time I've never felt the need) and mostly I see authors posting screenshots or screencasts to help reviewers with this.
> There is a sense that pull requests actually might make it too easy to comment on a line of code. This ease might be leading to the conflicts and hyper-zealous commenting that frustrates many in our industry.
If there are a lot of comments, I usually find its because of either a disconnect between author and reviewer that should have been addressed before authoring the code or there is something that needs to be automated.
[+] [-] vncecartersknee|4 years ago|reply
How often is this even feasible though? sure if there's little difference between the PR branch and whatever the last build I've done is, it wont take too long to build but then I have to install our product and get it set up to test. We have a ton of legacy code that isn't easily testable. We do have a CI build but it takes like ~6 hours to build.
[+] [-] ferdowsi|4 years ago|reply
[+] [-] moogly|4 years ago|reply
I much prefer to go in and check people's small commits to master than look at a huge PR before it gets merged in, but it requires you (and the whole team) to be really diligent and do it on a daily basis, and when it comes to big features, you lose a lot of the context. OTOH, the amount of times I've been presented with a huge PR of an entire feature where the author begins by saying "sorry for the huge PR" and my predictable reply "I'm having trouble getting the whole picture here, but LGTM?" are countless, so I'm not sure that's better either.
Not sure how to reconcile the two models into something great.
As for the style of code review, I ask the team and adjust accordingly. I can be "big picture guy", or I can do that and simultaneously be "Mr. Nitpick" if people so wish (some people actually do, me included).
[+] [-] dan-robertson|4 years ago|reply
[+] [-] irjustin|4 years ago|reply
Oh man I'm so guilty of this. If there's a spec, I basically say "YEP LOOKS GOOD!"
But shamefully, I've been bitten by this when the spec itself is written with the wrong logic. Argh!
[+] [-] marceloabsousa|4 years ago|reply
[+] [-] IvyMike|4 years ago|reply