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.
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. 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
Ah, I see that OP added more details while I was still writing mine. Specifically, the detail about having only a group of 5 fairly-experienced engineers.
In that case, the question still has to focus on what is an acceptable risk and how risk decisions are made. After all, that’s the other half of code reviews: first is to identify something that doesn’t work, and second is to assess if it’s impactful or worth fixing.
As I said before, different projects have different definitions of acceptability. A startup is more amenable to shipping some rather ugly code, if their success criteria is simply to have a working proof of concept for VCs to gawk at. But a military contractor that is financially on the hook for broken code would need to be risk-adverse. Such a contractor might impose a two-person rule (ie all code must have been looked at by at least two pairs of eyeballs, the first being the author and the second being someone competent to review it).
In your scenario, you need to identify: 1) what your success criteria is, 2) what sort of bugs could threaten your success criteria, 3) which person or persons can make the determination that a bug falls into that must-fix category.
On that note, I’ve worked in organizations that extended the two-person rule to also be a two-person sign-offs: if during review, both persons find a bug but also agree that the bug won’t impact the success criteria, they can sign off on it and it’ll go in.
Separately, I’ve been in an organization that allows anyone to voice a negative opinion during a code review, and that will block the code from merging until either that person is suitably convinced that their objections are ameliorated, or until a manager’s manager steps in and makes the risk decision themselves.
And there’s probably all levels in between those two. Maybe somewhere has a 3-person sign-off rule. Or there’s a place that only allows people with 2+ years of experience to block code from merging. But that’s the rub: the process should match how much risk is acceptable for the project.
Boeing, the maker of the 737 MAX jetliner that had a falty MCAS behavior, probably should use a more conservative process than, say, a tech startup that makes IoT devices. But even a tech startup could be on the hook for millions if their devices mishandle data in contravention to data protection laws like the EU’s GDPR or California’s CCPA. So sometimes certain parts of a codebase will be comparmentalized and be subject to higher scrutiny, because of bugs that are big enough to end the organization.
Thanks for the insight
In your scenario, you need to identify: 1) what your success criteria is, 2) what sort of bugs could threaten your success criteria, 3) which person or persons can make the determination that a bug falls into that must-fix category.
I think is a good part of what I needed to be told, thank you!
For example, #3
yes that’s what I meant, comments on the review would just be a suggestion to the author, which can possibly fix but not in a controlled (or closed-loop as you said) manner.
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
In my experience even a too long staging process for merge/review, can hinder development since people that work on the same things can need each other changes to move on, so how to know where to trace the line and merge? No breaking the build I would say is universally accepted, but what if for example an issue is internal to a WIP feature?
people that work on the same things can need each other['s] changes to move on
If this is such a regular occurrence, then the overarching design of the code is either: 1) not amenable to parallelized coding at all, or 2) the design has not properly divided the complexity into chunks that can be worked on independently.
I find that the latter is more common than the former. That is to say, there almost always exists a better design philosophy that would have allowed more developers to work without stepping on each other’s toes. Consider a small group designing an operating system. Yes, there have to be some very deep discussions about the overall design objectives at the beginning, but once the project is rolling, the people building the filesystem won’t get in the way of the UI people. And even the filesystem people can divide themselves into logical units, with some working on the actual storage of bits while others work on implementing system calls.
And even when a design has no choice but to have two people working in lock-step – quite a rarity – there are ways to deal with this. Pair programming is the most obvious, since it avoids the problem of having to swap changes with each other.
I’ve seen pair programming done well, but it was always out of choice (such as to train interns) rather than being a necessary mandate from the design. Generally, I would reject designs that cannot be logically split into person-sized quantities of work. After all, software engineering is ultimately going to be performed using humans; the AIs and LLMs can figure out their own procedures on their own, if they’re as good as the pundits say (I’m doubtful).
but what if for example an issue is internal to a WIP feature?
I forgot to answer this. The question is always: will this materially impact the deliverable? Will the customer be unhappy if they hit this bug?
If the WIP isn’t declared to be fully working yet, then sure, let it on the branch and create a ticket to fix this particular bug. But closed-loop requires making this ticket, as a reminder to follow it up later, when the feature is almost complete.
If instead the bug would be catastrophic but is exceptionally rare, then that’s a tough call. But that’s precisely why the call should involve more people, not less. A single person making a tough call is always a risky endeavor. Better to get more people’s input and hopefully make a collective choice. Also, humans too often play the blame-game if there isn’t a joint, transparent decision making process.
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.
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).
Wow. I’m more chill, I prefer #5, where some (no 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.
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…
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.
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.
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.
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.
Let’s just go full boar bore hypothetical:
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!
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.
mattreb
Eager Eagle
devfuuu
gsv
KairuByte
EffortlessGrace
Kissaki