Hey, I want your opinion on code reviews, what is the best way to use them in a professional environment? Pick one of the following and give me your thoughts (from the most forgiving to the most strict):
- no code reviews, they are useless
- optional code reviews
- mandatory reviews on code that is already merged, optional fixes
- mandatory reviews on code before merging (like a pull request), with a time-frame for optional fixes (i.e. whether to fix what has been pointed out is up to the author), merge will occur anyway.
- mandatory reviews on code before merging (PR) with mandatory fixes.
Of course in open source development with public contributions, you’ll often see (5), but I’m not convinced it could work in professional dev.
Edit: I’m talking about a team of 5 mid to senior devs (no junior or interns) working on a 2-3 year project without many security concerns, but feel free to give me your general opinion.
Why would the answer ever be anything other than 5?
Let’s just go full boar hypothetical: Someone is trying to merge malicious code. Anything other than 5 means the malicious code gets merged.
Let’s just go full
boarbore hypothetical:Whoop yeah good catch but I’ll leave it.
I can only think that what they have in mind is a small project with only few people involved. Otherwise anything other than 5 is ridiculous.
In a small shop where people really know and trust each other and all have high quality standards and would never break main - is code review necessary and for what purposes if so?
Yes, absolutely.
“Never break main” is the same concept as “never get in a car accident.” Good in theory, but it’s no replacement for insurance.
Everyone makes mistakes. PRs help catch those mistakes. Yes, bugs will still sneak in, no one is perfect, but a proper PR process is absolutely vital no matter the team size.
Even well intentioned people can make mistakes
Yes, it’s necessary. Even if everyone writes perfect, bug-free code, people learn from code reviews.
I work in a similar environment. Most of our projects lifecycle we were 4 devs who knew each other reasonably well and all had high trust in each other. Unless it was a one-liner or a hotfix at 4am did get merged without a code review. Most if the reviews were just hitting the checkmark, things look fine done. But sometimes someone makes a mistake, as is human. In that case we caught it before it hit main.
Anyone who answers other than 5 is a fucking idiot.
The main branch should always contain working code. You should assume you can deploy it to production and it will work. Not having anybody review your code is lunacy, and ignoring critical feedback is even more insane. You can ignore linting complaints assuming you don’t have a linter that fails the build, but you should not consider faulty business logic as an optional fix.
but you should not consider faulty business logic as an optional fix
The same goes for unreadable/unmaintainable code (though that’s a bit harder).
5 only. Even when I only had a single partner doing non-professional work. Though on occasion when life got away from us we would just merge after a few days. But as a rule we always had at least 24 hours.
deleted by creator
All code going to the main branch must have a corresponding pull request reviewed and approved by someone with knowledge of the codebase. You really shouldn’t have the front end guy approving backend code.
Ai doesn’t count as a code review.
At my previous job, the policy also said you were supposed to actually check out the code and run it locally. Found a lot of bugs and issues that way.
At my current job, it’s often a rubber stamp. I’ve seen things like “that’s too many parenthesis. This won’t run” sail through. This is bad.
There should also be automated tests and checks.
A long time ago a director told me “software engineers are the most sensitive people on the planet” and I think he was right. Some people just can’t take feedback. They take something like “please sort your imports. We agreed to use isort last week” as a personal attack.
They take something like “please sort your imports. We agreed to use isort last week” as a personal attack.
I would take this personally as well, to be honest. Using isort over Ruff? Blasphemy.
As discussed at length in last week’s planning meeting, we agreed to continue using isort at this time. Here is the decision document to review: {confluence link}. If you would like to relitigate the issue, which I would not recommend, please add it to the tech planning meeting agenda.
(More seriously, I started using ruff and have no complaints about it.)
I’ll also say 5 but I have my gripes with it. Mainly with the “review from any other engineer” aspect that usually comes with it… I have met so many engineers whose review seems to just depend on who created the MR, as opposed to what’s in it. When an MR with 500+ lines changed gets reviewed in about 10s after requesting it, it’s kinda obvious that the system is broken.
The people I’ve worked with who are good at their job and I’d probably be okay with them merging their changes without reviews would always ask for a review, even when it’s not mandatory or enforced. And their MR would already have comments by themselves around bits I might have a question around, and they’d even come with prompts of what they want input on. Whereas the people I wish wouldn’t even be allowed to approve anything would usually ask for an approval instead (even the wording seems telling). Sadly, often these 2 groups will have the same job title and HR will dictate that they should have the same permissions and say in things, which is what usually breaks the system IMO.
And lastly, the amount of people who seem to treat reviews as currency/favours and just rubber stamp each others MRs without looking…sigh.
From a scientific modeler perspective: Always trying to do 5 (or 4), but I’m having difficulties getting a culture of reviewing each other’s codes going. Many times I was asked to “just merge” months after submitting a PR. In the context of operational or large community codes, 5 is usually strictly enforced. Weather services don’t appreciate broken code.
when you were asked to “just merge” why was the review still on going? too large, or you didn’t find an agreement with the autor?
It was more a question of finding the time within the daily business of doing science. I suppose it is also a culture issue, the priority to do a review is usually low. In other words, there was de facto never a review.
Thanks, that’s actually interesting. I’ve found making #5 work with limited human resources / deadlines challenging, and wondered what to do. My answer in the past has been to lower review quality (reviewing faster) while keeping #5.
In my case the priority for reviews was high, but we were limited by the reviewers/developers ratio since most people would not do reviews…
I’m glad to hear that lack of time/resources for code reviews are more common. Also for clarification: I was the author and requested reviews by my colleagues. In reverse, I did not receive requests to review PRs so far. Tbh. I would really like such a review culture as it is already standard in scientific publishing and it would have avoided some obvious bugs we did encounter in the past. Having that said, as I did not receive any review at all and I would appreciate low(er) quality reviews better than none.
5 with reasonable acceptance and use, even advocacy, for up to 1. I don’t see a difference between 4 and 5, though.
Reviews should be the norm. Even for simple changes, a simple code change should be simple to review and approve, too. At the same time, some formatting changes or small or minimal changes with high confidence can be pushed to main without review - that’d be just wasted time and effort on the reviewer’s side. High urgency can also warrant an immediate push to main, or live hotfixing on prod if possible, with a corresponding PR still open.
Wow. I did’t expect my team to be at the more relaxed end of the responses!
We have #5, but some (non-breaking) feedback can still be deferred to a future follow up issue.
That said, my team rarely exercises the option to defer feedback.
We have #5, but some (non-breaking) feedback can still be deferred to a future follow up issue.
This is usually my preferred option, but usually i differentiate between blocking and non-blocking feedback in my reviews. Non-blocking is some improvement that can be made, but is not necessary, like cleaning up some (tangentially-related) code. Blocking is anything that is logically incorrect, unreadable, uses deprecated features unnecessarily, etc.
