4 ms·

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev t

I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will be motivated to understand it (and potentially improve it). By contrast, with code reviews often the reviewer does not have time to really deep dive into the code and will only have a superficial understanding of it. Oh, also use code quality scanners to keep an eye on tactical code debt.

rbongers 4y ago
I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.
kentonv 4y ago
The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?
- partdavid 4y ago
  It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.
- aftbit 4y ago
  In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.
JamesBarney 4y ago
I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find. 98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug. when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back and forth unless there is a clear mentor/mentee relationship.
- Arainach 4y ago
  This is exactly backwards. One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who should be the one keeping their mouth shut.
  - JamesBarney 4y ago
    Usually the feedback is not "I don't understand this" or "I find this unclear" it's "I think it would be clearer if you did it this way instead of this way". Or "if you made this change it would make it more maintainable". And if everyone followed everyone else's advice you end up with everyone writing code in their reviewers style instead of their own which doesn't seem like a gain. Not to mention code changes have a cost, very few devs spend as much time testing and thinking about their code when making code changes. So many times you are trading more thought out, better tested code for less tested less thought out code. Because of this I've seen countless bugs introduced from code changes related to trivial PR requests.
    - Arainach 4y ago
      If bugs are being introduced, that means the team's testing culture is poor. If someone asks me to make a change in code review, I know that my modifications are fine because the unit tests still pass (and the integration tests, but it's rarer to have to touch those). If I forget to manually run them the build system runs them before my code is let in. Writing in "the reviewer's style" is preferred because they are the reader. For most things that are "style" there should be explicit style guides; for the things that aren't, the reader's opinion is better every time. If your team isn't getting useful code review feedback, that's also a team culture problem and your senior eng need to step up and lead by example. "I don't understand this" is important feedback. At a minimum, more comments are needed, but it probably also signals that variable or method names need improvement. My experience (15+ years, companies of all shapes and sizes from college shops to FAANG) is that the kind of engineer who feels that code review feedback isn't worth listening or is a waste of time to is detrimental to the team in both the short and long term.
cableshaft 4y ago
So you're advocating for pair programming all of the time? As someone who has to do both pair programming and code reviews quite a bit this past year (mentoring some quite junior developers), I find pair programming much more mentally exhausting. Two hours of that and my mind is pretty much spent for the day.
- lmm 4y ago
  IME PR reviews are generally a half-assed substitute for pair programming. I don't really enjoy doing pair programming, but can't deny the effectiveness.
- aftbit 4y ago
  I don't read the parent as advocating for pair programming, just that any given part of the code should be understood by at least two people. This is possible to achieve even on a fully asynchronous team.
wahnfrieden 4y ago
you'll have compliance needs to speak for if you let code to prod unreviewed

rbongers 4y ago

I have been on teams that never review code and teams that always review code, and I can confidently say I never want to work on the former again. That was with a junior team, but your team is going to have new people, if not junior, at some point. People who are familiar with each other's code and have agreed on standards can review code pretty quick. I would rather have it and not need it than need it and not have it.

kentonv 4y ago

The most important reason for code review is not to fix the code but rather to transfer knowledge between developers. I've learned a lot of techniques for writing better code from suggestions from my code reviewers, and from reviewing other people's code. Without code review, when will a junior developer ever learn anything from a senior developer?

partdavid 4y ago
It works both ways, too--it makes more developers aware of the others' work, what changes are being made and so forth. So it's not just expertise that gets transferred, but also the current state of the codebase.
aftbit 4y ago
In that case, it doesn't matter if you do the code review before or after they merge and deploy. You could read through the diffs in the commit log whenever it is convenient for you, and leave review comments for the other developer on their PR after the fact.

JamesBarney 4y ago

I've found PRs are usually a waste of time because people focus on such trivial bullshit because they're easy to find. 98% of the review comments I receive hark back to some handwavy explanation about maintainability. But never in any way that could actually cause a bug. when I run teams my rules with PRs are if it's not a bug, and it's not against the code review guidelines leave it. It's usually not worth the back and forth unless there is a clear mentor/mentee relationship.

Arainach 4y ago
This is exactly backwards. One person writes the code. Many people read the code. If something is unclear or feels wrong to the reader, they get priority. Reviewers should leave all sorts of feedback; the response of the author should be to just implement the feedback unless they can clearly articulate why doing so is not a good idea. You're right that "it's not worth the back and forth", but completely wrong on who should be the one keeping their mouth shut.
- JamesBarney 4y ago
  Usually the feedback is not "I don't understand this" or "I find this unclear" it's "I think it would be clearer if you did it this way instead of this way". Or "if you made this change it would make it more maintainable". And if everyone followed everyone else's advice you end up with everyone writing code in their reviewers style instead of their own which doesn't seem like a gain. Not to mention code changes have a cost, very few devs spend as much time testing and thinking about their code when making code changes. So many times you are trading more thought out, better tested code for less tested less thought out code. Because of this I've seen countless bugs introduced from code changes related to trivial PR requests.
  - Arainach 4y ago
    If bugs are being introduced, that means the team's testing culture is poor. If someone asks me to make a change in code review, I know that my modifications are fine because the unit tests still pass (and the integration tests, but it's rarer to have to touch those). If I forget to manually run them the build system runs them before my code is let in. Writing in "the reviewer's style" is preferred because they are the reader. For most things that are "style" there should be explicit style guides; for the things that aren't, the reader's opinion is better every time. If your team isn't getting useful code review feedback, that's also a team culture problem and your senior eng need to step up and lead by example. "I don't understand this" is important feedback. At a minimum, more comments are needed, but it probably also signals that variable or method names need improvement. My experience (15+ years, companies of all shapes and sizes from college shops to FAANG) is that the kind of engineer who feels that code review feedback isn't worth listening or is a waste of time to is detrimental to the team in both the short and long term.

cableshaft 4y ago

So you're advocating for pair programming all of the time? As someone who has to do both pair programming and code reviews quite a bit this past year (mentoring some quite junior developers), I find pair programming much more mentally exhausting. Two hours of that and my mind is pretty much spent for the day.

lmm 4y ago
IME PR reviews are generally a half-assed substitute for pair programming. I don't really enjoy doing pair programming, but can't deny the effectiveness.
aftbit 4y ago
I don't read the parent as advocating for pair programming, just that any given part of the code should be understood by at least two people. This is possible to achieve even on a fully asynchronous team.

wahnfrieden 4y ago

you'll have compliance needs to speak for if you let code to prod unreviewed