Programming

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.

There's a number 6, where if you're on a small team with good synergy and well experienced members, the PR is a good thing each member requests to answer the question , hey can you double check my work and please give me feedback because I really trust your judgement.it's a no nonsense way for confident developers to get a second opinion.

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.

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.

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.

5. 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.

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.

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.

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.

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.

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.

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?

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.

With regards to the given list, I think #2 would be the most forgiving, in the sense that #1 suggests that code reviews are viewed solely negatively and are punishable if undertaken. But that minor quibble aside, I have some questions about what each of these would even look like.

For example, #3 seems to be that code can be committed and pushed, and then review sought after-the-fact, but any results of the code review would not be binding for the original commit author to fix, nor apparently tracked for being fixed later. If that's a correct description, I would describe that as the procedurally worst of the bunch, since it expends the effort to do reviews but then has such an open-loop process that the results of the review can be swept under the rug.

On the note of procedure, it is always preferable to have closed loops, where defects are: a) found, b) described, c) triaged, d) assigned or deferred, e) eventually fixed, and f) verified and closed out. At least with your examples #1 and #2, they don't even bother to undertake any of the steps for a closed loop. But #3 is the worst because it stops right after the first step.

Your example #4 is a bit better, since in order to keep to a specified timeframe, the issues found during review have to at least be recorded in some fashion, which can satisfy the closed-loop steps, however abbreviated. If the project timeline doesn't allow for getting all the code review issues fixed, then that's a Won't Fix and can be perfectly reasonable. The issue has been described and a risk decision (hopefully) was made to ship with the issue anyway. All things in life are a balance of expected risk to expected benefit. Ideally, only the trivial issues would be marked as Won't Fix.

But still, this means that #4 will eventually accumulate coding debt, probably quite quickly. And as with all debt, interest will accrue until it is either paid down or the organization succumbs to code insolvency, paralyzed because the dust under the rug is so large that it jams the door shut. I hope you'll allow me to use two analogies in tandem.

Finally, there is #5 which is the only one that prevents merging code that still has known issues. No doubt, code that is merged can still have further bugs that weren't immediately obvious during code review. But the benefit is that #5 maintains a standard of functionality on the main branch. Whereas #4 would wilfully allow the main branch to deteriorate, in the name of expediency.

No large organization can permit any single commit to halt all forward progress on the project, so it becomes imperative to keep the main branch healthy. At a minimum, that means the branch can be built. A bit higher would be to check for specific functionality as part of automated checks that run alongside the code review. Again, #4 would allow breaking changes onto the branch due to expediency, whereas #5 will block breaking changes until either addressed, abandoned, or a risk decision is made and communicated to everyone working on the project to merge the code anyway.

TL;DR: software engineering processes seek to keep as many people working and out of each other's way as possible, but it necessarily requires following steps that might seem like red-tape and TPS reports