Code reviews: A success story

He's obviously self-praising for a promotion or looking for another job. I don't think it should be taken seriously on the matter of the effectiveness of code-reviews

Disagree. It's only the author's fault. We can't expect engineers to write code and find every last bug in other people's code especially when PRs can be thousands of lines to review.

A broken culture would be expecting engineers to find every last bug in other people's code and blaming them for bugs.

The broken culture is what you actually suggested and it has a name, the blame culture.

Breaking down the size of the change is truly important, otherwise it's easy to miss things and to also disregard them as little details when wanting to avoid blocking the whole change on a "small" thing (which may only seem small because the PR is now huge)

I miss Phabricator from my time at Meta so much, I made this to achieve a Phabricator-like stacked-commit experience via the git cli: https://pypi.org/project/stacksmith/

Don't worry, he also asked his own reviewee, who said the reviews were helpful and in no way obnoxious.

But he does back it up with actual facts (as far as we can trust the author to tell the truth) - the feature that the author gave feedback on shipped without any issues. (The article actually doesn't say whether A was fixed-and-bug-free before B shipped, but it certainly sounds like B was less stressful to ship.)

Yes, these were my reviews. But this is from the point of view of the reviewee. I’m telling the story of what this person felt and later told me.

Shameless plug, but we built http://CodePeer.com - to bring Critique like features to everyone else. Take it for a spin if you like!

Former Googler. I also miss Critique/Gerrit. I've tried a bunch of alternatives, and I like CodeApprove:

https://codeapprove.com/

It's great if you have a team that does code reviews. It works less well for reviewing contributions from external contributors on an open-source project,a as the contributor likely just wants to get their PR merged and doesn't want to learn a special reviewing tool.

No affiliation, just a happy customer.

Github's code review tool is uniquely bad. Notably it presents every comment as blocking and requiring sign off - even a "Glad someone cleaned this up! <thumbs up emoji>" needs clearing before merge.

It also has some UX snafus that cause reviewers to write a number of comments and then forget to publish any of them, leading to a lot of interactions along the lines of "I thought you were going to review my PR?" "I already did?"

Shameless plug but since you asked ... CodeApprove (https://codeapprove.com) is probably the closest thing you can get to Critique on GitHub. It doesn't help with the Piper/CitC/Code Search parts though, and I agree those were excellent.

I don't think I ever try to verify the correctness of the code in code review. That seems hopeless to me. The original developer should be assumed to produce working code, which is covered with tests and maybe will go through QA (if you have that). Of course there are occasional bugs you can spot, but the main goal is to review the architectural approach and code clarity. You can also note if something isn't properly covered with tests. If you managed to make any security, performance, or edge case handing suggestions, that's a nice bonus.

Maybe we're in a very different context, but to me if you can't understand what the code does without a debugger, then you ask the author to make the code clearer.

Thankfully I'm not working with you

Mandatory code review definitely creates red tape. Every place I've been with mandatory code review, I always see people "looking for a stamp".

At my current job, code review requirements are set on a per-folder basis, with many folders not requiring a review. People ask for a review because they want a review (or, sometimes, they dont. For example, I don't ask someone to review every quick one-liner of the systems I am an expert in).

Same.

> Code reviews have a bad rap: they are antagonistic in nature and, sometimes, pure red tape.

I wonder if folks know that this is a job? What are you gonna do, not do it? Cry at night because you forgot for the hundreth time to add token strings to translation files and not be hard-coded? Come on.

You would be surprised! I have encountered the attitude that code reviews are a waste of time. It's not common, and I have never seen this attitude "win" across a team/company but it definitely exists. Some engineers are just overconfident, they believe they could fix everything if everyone would just let them code.

Then you are very lucky. I definitely have met those sorts. I’m even aware of teams that collectively push to the main brain under the promise that they’ll probably look at each other’s code later, maybe.

I saw no proof of the later review for all pushes.

I’ve never worked somewhere where mandatory PR reviews didn’t turn into mostly red tape.

The pressure to get work done faster in the long term always wins out over other concerns and you end up with largely surface level speed reviews that don’t catch much of anything. At best they tend to enforce taste.

In 20 years across many companies and thousands of PRs, I’ve never had a reviewer catch a single major bug (a bug that would have required declaring an incident) that would have otherwise gone out. I've pushed a few major bugs to production, but they always made it through review.

I’ve been doing this since well before everyone was using GitHub and requiring PR reviews for every merge. I haven’t noticed a significant uptick in software quality since the switch.

The cost is high enough that I’d like to see some hard evidence justifying it. It just seems to be something people who have never done any different take on faith.

from this thread "Mandatory CR is mostly a way for mediocre programmers to feel like they're contributing IMHO."

Are you using AI for public contributions? Or a private repo? How does it deal with things like project conventions? e.g. "Currency should be represented using a Money class", "This method should use our utility class", etc. Do you upload the entire project's source code?