4 ms·

Yes, they are a giant waste of time and in general slow things down and don’t improve your ability to add more features over time. Bigger win from having good t

Yes, they are a giant waste of time and in general slow things down and don’t improve your ability to add more features over time. Bigger win from having good tests and properly factored code encourage by shared owership (no disincentives to change any code you see fit)

01100011 8y ago
Depends on the team. Sometimes they're great, sometimes people just start rubber-stamping the reviews because of time pressures, laziness, or lack of familiarity with the code. Right now, I'm on a team of 3. 2 embedded devs and 1 web dev. The web dev keeps asking me to PR their web framework changes, but I'm not a web guy. The other embedded dev has ADHD and can only rubberstamp my reviews. Basically I'm just wasting time complying with the branch/PR/merge/tag cycle.
- Yen 8y ago
  I've had a similar problem in the past. The things I've found to be very helpful are: 1) Don't look at the PR diff as a whole, look at individual commits, and judge each one, then read the PR diff. 2) If the commits are too big to keep in your head, don't make individual sense, etc., ask your coworker to rewrite their history until you can do 1) 2b) If the individual commits make sense, but the PR still is too big to comprehend, this PR should probably be split into multiple smaller PRs which can be reviewed, approved, and released individually (or at least in sequence). 3) If you lack still lack enough context to make a good review, ask your coworker to walk you through the commit, and just explain out loud why they're making the changes they're making. Sometimes, a "rubber-duck code review" is enough. Even if it's not, any programmer can read and understand a single line of code at a time, and you can likely tell if that line of code is complex or elegant. Plus, even if you don't know the full technical context of a proposed change, people are fairly good at picking up whether another person has an explanation that makes sense, or if they seem to be making it up as they go. 4) Spend the time and do the reviews, and become more familiar with that part of the code. It sounds like right now, that part of the code has a bus factor of 1. 5) Track time spent doing code reviews, and track bugs caught during code review. Code review has notable benefits for your team/company, so justify and sell that. You have fewer bugs & a better bus factor. When estimating your next sprint (or other unit of work), know that you'll spend X hours per week doing code review, and integrate that.
  - andymoe 8y ago
    I hear you, I’m just one of those pair programming zealots now. Whenever we work with clients who insist on PR workflow eventually we just bury them in PRs and it basically slows velocity to a crawl until we sort out the ownership and start committing to master and get proper piplined ci in place. Or we work with folks who have historically done PRs and think they are helping code quality but everything is a mess because there’s so much resistance to refactoring as part of normal work. No one wants to accept massive changes in PRs.
benji-york 8y ago
You might like this talk about code reviews from an art critique perspective: https://www.youtube.com/watch?v=lpWvYxEW09k https://www.youtube.com/watch?v=lpWvYxEW09k

01100011 8y ago

Depends on the team. Sometimes they're great, sometimes people just start rubber-stamping the reviews because of time pressures, laziness, or lack of familiarity with the code. Right now, I'm on a team of 3. 2 embedded devs and 1 web dev. The web dev keeps asking me to PR their web framework changes, but I'm not a web guy. The other embedded dev has ADHD and can only rubberstamp my reviews. Basically I'm just wasting time complying with the branch/PR/merge/tag cycle.

Yen 8y ago
I've had a similar problem in the past. The things I've found to be very helpful are: 1) Don't look at the PR diff as a whole, look at individual commits, and judge each one, then read the PR diff. 2) If the commits are too big to keep in your head, don't make individual sense, etc., ask your coworker to rewrite their history until you can do 1) 2b) If the individual commits make sense, but the PR still is too big to comprehend, this PR should probably be split into multiple smaller PRs which can be reviewed, approved, and released individually (or at least in sequence). 3) If you lack still lack enough context to make a good review, ask your coworker to walk you through the commit, and just explain out loud why they're making the changes they're making. Sometimes, a "rubber-duck code review" is enough. Even if it's not, any programmer can read and understand a single line of code at a time, and you can likely tell if that line of code is complex or elegant. Plus, even if you don't know the full technical context of a proposed change, people are fairly good at picking up whether another person has an explanation that makes sense, or if they seem to be making it up as they go. 4) Spend the time and do the reviews, and become more familiar with that part of the code. It sounds like right now, that part of the code has a bus factor of 1. 5) Track time spent doing code reviews, and track bugs caught during code review. Code review has notable benefits for your team/company, so justify and sell that. You have fewer bugs & a better bus factor. When estimating your next sprint (or other unit of work), know that you'll spend X hours per week doing code review, and integrate that.
- andymoe 8y ago
  I hear you, I’m just one of those pair programming zealots now. Whenever we work with clients who insist on PR workflow eventually we just bury them in PRs and it basically slows velocity to a crawl until we sort out the ownership and start committing to master and get proper piplined ci in place. Or we work with folks who have historically done PRs and think they are helping code quality but everything is a mess because there’s so much resistance to refactoring as part of normal work. No one wants to accept massive changes in PRs.

benji-york 8y ago

You might like this talk about code reviews from an art critique perspective: https://www.youtube.com/watch?v=lpWvYxEW09k https://www.youtube.com/watch?v=lpWvYxEW09k