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.
5 works best. In order to do 5 well, PRs must be small and nothing in any given PR should really be a surprise to anyone on the team. To accomplish both of these, this means that the team already discussed the work, broke it down into tasks, and then pointed the work. If any tasks are going to take longer than a few days, break it down. Otherwise, you’ll end up with 100 file PRs and 2 second LGTM reviews.
I’ve worked for managers that didn’t believe in the necessity of code reviews, and some that downright hated them. All of them wrote garbage code, and the codebases were equally awful. Some people just cannot handle even the slightest critique.
If you find yourself on a dev team doing anything other than 5, run, don’t walk, and find a new job if you can. If you can’t, focus on increasing your knowledge and skills, because no one else there is going to help you.
If you find yourself on a dev team doing anything other than 5, run, don’t walk, and find a new job if you can. If you can’t, focus on increasing your knowledge and skills, because no one else there is going to help you.
Is great advice, which I have followed, and it has served me well!
Thank god you specified dev team. I personally just go by ear and I mainly just work alone
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?
Even well intentioned people can make mistakes
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.
Yes, it’s necessary. Even if everyone writes perfect, bug-free code, people learn from code reviews.
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 don’t think I’ve ever not done #5 in all places I’ve worked. It’s insane to even think otherwise. What happens is that if anything really urgent needs to bypass the process, then the team lead or someone in high power can merge or bypass the normal procedure, but that’s rare.
Ok then you’re moving the problem to the reviewer, what change request would you say is a mandatory fix? In the place you worked, were you marking the comments on the review in any way to make them mandatory / non mandatory? Or just discussing them one by one?
Edit: given the amount of downvotes let me clarify this comment is not trying to be polemical, I’m genuinely asking the above questions.
Everything gets reviewed. If you have a constructive comment you put it in the review.
I note when I think I have a better way of doing something but the existing way works fine, and I leave that fix up to the submitter. But sometimes I just say no this is wrong. And then whether it gets merged anyway depends on my role. I’m a tech lead now so that’s basically the end although if they want to argue their case I’ll hear them out.
The devs I work with are all seniors and have all been working in the system longer than me, so I respect them and listen when they disagree. Generally when that happens I’m right in principle and they agree with me, but the code is a fucking mess and we can’t do A right without having to change the rest of the alphabet, and we have bigger fish to fry.
In other positions I made my comments and whoever was in charge got to decide whether to accept the change or send it back. I try to always make at least one constructive comment even if it’s just like: I really like how you did this.
I’m not sure what problem you think is being moved to the reviewer. It’s a team and everyone has the same end goal. I appreciate when my code is reviewed because any of us can make a mistake or forget to consider some outside factor. Code review is where assumptions are tested and discussed and hopefully everyone comes away knowing more and agreeing on a path forward.
Everything gets reviewed. If you have a constructive comment you put it in the review.
I’m a tech lead now so that’s basically the end although if they want to argue their case I’ll hear them out.
So basically you’re saying that all the comments are welcome, and should be argued, but you have to appoint a person that have the final say on what to merge right?
I’m not sure what problem you think is being moved to the reviewer.
The problem of deciding what should be merged or blocked, I’ve mainly made this post to understand that, so saying “#5 or you’re crazy” doesn’t answer much. But I’ve probably worded that badly.
If a story gets created, the code will be merged… when it’s right. If you’re talking OSS, then I am out of my element, but I’ll wager there’s no universal answer because each code owner sets their own standards.
you have to appoint a person that have the final say on what to merge right?
If there is work that needs to be done, and you are asked to do it, the code will be merged when it’s right. I don’t decide what to merge, I decide when something is ready to merge.
The problem of deciding what should be merged or blocked
If you want to merge something and I read it over and reject the PR because you forgot about concurrency, that doesn’t mean you don’t get to merge, it means that it’s not finished baking. And assuming you give a shit about the code your response should be “oh shit, lemme fix that and resubmit” OR “actually this code will never have concurrent access, and here’s why.”
You’re making this process sound adversarial when it isn’t. It’s a group effort. Everyone wins or loses together.
The problem of deciding what should be merged or blocked
If you want to merge something and I read it over and reject the PR because you forgot about concurrency, that doesn’t mean you don’t get to merge, it means that it’s not finished baking. And assuming you give a shit about the code your response should be “oh shit, lemme fix that and resubmit” OR “actually this code will never have concurrent access, and here’s why.” You’re making this process sound adversarial when it isn’t. It’s a group effort. Everyone wins or loses together.
You’re obviously right, you’re misunderstanding me. With blocked I didn’t meant “forever”, I meant until the issues is discussed. I’m merely asking how the reviewer is making the call on what should be addressed before merge in #5
If there is work that needs to be done, and you are asked to do it, the code will be merged when it’s right. I don’t decide what to merge, I decide when something is ready to merge.
you’re really nitpicking my English, which I know is not great, but yeah that’s what I was asking, so you use #5 with a single person making the final call on when something is ready.
You’re making this process sound adversarial when it isn’t.
Absolutely not, I’ve never said or thought that. But of course development is made of people and computers can only tell you what compiles, not “when it’s right” as you say. So of course I’m more curious to understand how to solve situations when you do have conflict, if you don’t it’s easy.
you’re really nitpicking my English
Mate, the hardest part of software development is communication and autism is ubiquitous — got a touch myself. So I over explain and I’m very specific. But let me make one thing abundantly clear: I don’t have time to spend this many words trying to be a pedantic asshole. I have much better things to do with my time. If you don’t like my approach, feel free not to engage, but I’m here trying to distill some value for you out of my experience.
Now, I don’t put too much stock in up and down votes (and to be clear none of your downvotes is from me — I don’t waste time responding to stuff I downvote), but the pattern suggests that what I’ve said resonates with other developers.
So of course I’m more curious to understand how to solve situations when you do have conflict, if you don’t it’s easy.
So here’s the disconnect: you’re worrying about shifting burdens like it’s a huge weight, but conflict is exceedingly rare — too rare to worry about. It’s a non-consideration.
I’m going to leave it there because I think anything else would just be repeating myself.
- It takes discipline to produce good code.
Anyone who answers other than 5 is a fucking idiot.
mandatory reviews on code before merging (PR) with mandatory fixes.
This one. Open PR, review by at least one peer, address concerns, merge.
Code review is not punishment - it’s part of your job. You should be willing and able to provide meaningful feedback to your peers. It also gives the team an opportunity to see how other people write code and to agree on norms and standards.
Always have done 5
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.
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?
As with a lot of things in life, it depends.
I use 1-5 depending on the repo, who made the change, what the change is about, and how involved I am in the project.
Though the “time-frame” idea of #4 is usually replaced by conversations if it’s a coworker, as it’s more effective.
Q: about #3, do you mean on code that is already merged / committed to the default branch?
Added a few details in the post. Of course it depends, but let’s say you’re the team lead and you have to fix a general rule (otherwise no one is going to do them) which one you’re more likely to go for? e.g. if you choose (2), it’s up to every single member.
Q: about #3
yes already merged, updated the post.
The way I see it, for any code review there are going to be different levels of recommendation regarding the comments. When I review, I try to make it clear what’s optional (/ nitpick) and what I’d really like to see fixed before I can approve it.
So even making some assumptions, I can’t choose between 4 and 5 because optional and “less optional” changes are often in a same PR.
The only one I haven’t done much of is #3. That one looks better if one has questions about code that was already reviewed, merged, and it’s likely in production.
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.
