27 ms·

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 tes

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.

hnlurker22 2y 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 2y ago
  None of the two were at play when I wrote this. Try again.
  - deleted 2y ago
    [deleted]
kyt 2y 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 2y 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.
- thedufer 2y 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 2y 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 2y 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).
- aaomidi 2y ago
  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.
- gunian 2y ago
  Idk much about other industries what would this logic look like in aviation or pharma?
- jasonpeacock 2y 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.
halayli 2y ago
The broken culture is what you actually suggested and it has a name, the blame culture.
- gunian 2y ago
  Let's play the blame game, I love you more Let's play the blame game for sure

hnlurker22 2y 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 2y ago
None of the two were at play when I wrote this. Try again.
- deleted 2y ago
  [deleted]

kyt 2y 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 2y 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.
thedufer 2y 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 2y 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 2y 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).
aaomidi 2y ago
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.
gunian 2y ago
Idk much about other industries what would this logic look like in aviation or pharma?
jasonpeacock 2y 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.

halayli 2y ago

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

gunian 2y ago
Let's play the blame game, I love you more Let's play the blame game for sure