Item — HN Reader

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.

3 comments

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.

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.

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.

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.

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

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.

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

Google has a culture of small PRs. If your PR is too big (think more than 500 lines changed) you’ll actually not be able to get readability reviews on them - unless someone in your team does the readability review for you.

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

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

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

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