Hacker Remix

Code reviews: A success story

64 points by mu0n 5 days ago | 60 comments

jasonpeacock 3 days ago

While I am a proponent of code reviews, what this article actually described is mentoring of a junior engineer by a senior engineer, and requiring effective testing to be implemented alongside the feature.

It also shows a broken culture where the other reviewers were not engaged and committed to the success of the feature. When a bug escapes, it's both the author _and_ reviewer at fault.

kyt 3 days ago

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.

Arainach 2 days ago

When issues happen, it is almost never just one person's fault.

Just trace the Whys:

1) Why did the bug happen? These lines of code 2) Why wasn't that caught by tests? The test was broken / didn't exist 3) How was the code submitted without a test? The reviewer didn't call it out.

Arguing about percentages doesn't matter. What matters is fixing the team process so this doesn't happen again. This means formally documented standards for test coverage and education and building team culture so people understand why it's important.

It's not "just the author's fault". That kind of blame game is toxic to teams.

jasonpeacock 17 hours ago

That's the difference between "your code & my code" vs "our code".

In a good culture it's "our code" where everyone shares the responsibility for quality, performance, and functionality.

thedufer 2 days ago

Our code review process involves a reviewer explicitly taking ownership of the PR, and I think it's a reasonable expectation with the right practices around it. A good reviewer will request a PR containing 1000s of lines be broken up without doing much more than skimming to make sure it isn't bulked up by generated code or test data or something benign like that.

aaomidi 2 days ago

And just to add to this, at least in Google generated code is never seen in a code review. That’s all just handled by Bazel behind the scenes.

thedufer 2 days ago

Ah, we draw a distinction between checked-in generated code and JIT generated code, and the former does show up in code review (which is sometimes the point of checking it in - you can easily spot check some of it to make sure the generator behaves as you expect).

gunian 1 day ago

Idk much about other industries what would this logic look like in aviation or pharma?

halayli 2 days ago

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

gunian 1 day ago

Let's play the blame game, I love you more Let's play the blame game for sure

hnlurker22 3 days ago

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

jmmv 2 days ago

None of the two were at play when I wrote this. Try again.

frankfrank13 3 days ago

> I also pushed for breaking large changes into smaller commits, because it is impossible to do a thorough review otherwise.

I've found this to be key for both giving and receiving feedback. Even small breaking commits are better in a lot of cases because they invite feedback in a way that 1000+ lines don't.

findjashua 3 days ago

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/

sqwxl 3 days ago

This looks really interesting. I've been using a similar tool called spr https://github.com/spacedentist/spr for the last six or so months at work. I really like the stacked diff/PR workflow. But spr has a lot of rough edges and im on the lookout for a better alternative.

Do you happen to know how your tool compares to spr? How production-ready is it?

findjashua 2 days ago

I had checked out SPR, but found the workflow based around reordering history via interactive rebase unintuitive & clunky. That was the motivation behind building this on my own.

I have been using this for a few months now, and it has served me well! I haven't spent much (any) time marketing it, so haven't really had any feedback from other users yet. Feel free to check it out & lmk if you have any suggestions. It's also open source, so feed free to open issue/PR on Github

dietr1ch 3 days ago

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)

hnlurker22 3 days ago

The author describes how his code reviews that he gave others are successful from his own point of view.

pavel_lishin 3 days ago

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

IMTDb 2 days ago

It’s also a biased view. The author admit that the feature he was involved in took longer to ship initially. Depending on the environment this can be an anti-pattern; don’t we say “release early, release often”.

In the same vein; the author says that the other feature took several releases to be stable. Were the other release purely bug fixes or did that give the engineer a chance to get early feedback and incorporate that into the feature ?

It’s clear that the author prefers a slow and careful approach, and he judges “success” and “failure” by that metric. It sometimes is the right approach. It sometimes isn’t.

jmmv 2 days ago

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.

awkward 3 days ago

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

jmmv 2 days ago

I did not ask. This is feedback that the reviewee gave me years after we both left Google.

hnlurker22 3 days ago

My statement still stands true regardless. Not worried.

strongpigeon 3 days ago

Only somewhat related, but I'd pay decent money to have access to the whole Piper/CitC/Critique/Code Search stack. As much as I've tried to like it, I just don't really like Github's code review tool.

awkward 3 days ago

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?"

plorkyeran 3 days ago

Requiring every comment to be resolved is not a standard part of GitHub’s code review system. That is something which your organization has gone out of its way to require.

awkward 2 days ago

This has been consistent across four organizations including one I participated in setting up. They don't seem to have gone far out of their way.

etothet 2 days ago

Overall, I personally find the experience better than Gitlab's merge request UI/UX.

catlover76 19 hours ago

[dead]

mtlynch 3 days ago

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.

tantalor 3 days ago

"lookup table of similar technology and services to help ex-googlers survive the real world"

https://github.com/jhuangtw/xg2xg

racl101 3 days ago

Do you know if this works with Azure DevOps? I hate their UI. At this point I'd love to use Github. But for some reason the higher ups want us to be on Azure DevOps.

codeapprove 3 days ago

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.

aaomidi 2 days ago

How does this handle force pushes and stacked PRs?

tristanb 3 days ago

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

aaomidi 2 days ago

How does this handle force pushes and stacked PRs?