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.
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.
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?
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 feature 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.
But where would all these people convene to make a collective choice? How about during code review?
If this is such a regular occurrence, then the overarching design of the code is either: 1) not amenable to parallelized team 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).
TL;DR: a design that requires lock-step development with other engineers probably is a bad design