5 ms·

Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another

Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have fallen so much in love with the idea.

the_gipsy 5y ago
> Not sure why we have fallen so much in love with the idea. Because organisations obsess about risk minimisation.
cbsmith 5y ago
That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together.
- miltondts 5y ago
  > That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? The code review process I'm familiar with is: open a PR, wait for a couple of reviewers to comment on the work. Then normally what happens is: - The comments are trivial, so I implement the changes (these are almost always things that could be automated, but for one reason or another haven't been). - The comments need to be discussed/clarified, so I need to call the person. The code by itself doesn't have all the context, all the things that were tried etc. A code walkthrough is a session with other coders, where it is explained how and why we arrived at the solution, what considerations were made and so on. Walkthrough sessions provide much faster feedback and all improvements can be discussed immediately with much greater clarity.
  - cbsmith 5y ago
    Got it. So for my semantics, your review process for PR's sucks, lacking engagement, and you have to have meetings to get it done right.
beebmam 5y ago
This is a deeply unpopular opinion, but I find myself aligning with this more every day. I think code reviews are a fundamentally broken process. Design reviews, before any (non-prototype) code even gets written, are vastly more important than code reviews in my opinion.
01100011 5y ago
Upvoted because these are all great points that I agree with but code review is still worth the trouble in many situations. On my team it helps us keep a consistent style and forces more people on the team to stay up to date with changes. Code quality is improved because of our review process. Pair programming would accomplish some of that, but not all team members would have a voice. Further, reviews are asynchronous. Sure, reviews are a time sink. No one likes them. We are still better off having done them.
valenterry 5y ago
I agree. Code reviews are still a nice "sanity check" where I don't have to understand in full detail what happens and I don't criticize style problems and minor issues. However, I check if the change fits into the bigger picture (in terms of architecture), that the intention of the changes is correct and that tests are added if needed. Hence, a combination of intense pair programming and knowledge sharing sessions in combination with quick & lightweight PR reviews as well as regular refactorings has worked best in my experience.
TheCoelacanth 5y ago
None of those are the type of things that code reviews should be focused on catching. They should be focused on things like: - Does the code make sense to a human? - Is this the best way to achieve the objective? - Is the thing that the code is trying to do even something that should be done?
- rixed 5y ago
  Those last two are the most important (and less subjective), and should definitively happen before the code is even written, using design docs.
dragonwriter 5y ago
> Most style issues should be caught by automated tools Some style issues can be, others (“do variables, functions, etc. have meaningful, accurate, relevant names?”) can’t be, until the automated tools are backed by AGI, but at that point “code review” and “automated analysis” become...not so different.
omegalulw 5y ago
> To bring someone up to speed on your chosen style, pair programming is much faster than code reviews Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.
twistedpair 5y ago
> - Most functional issues should be caught by tests (written by another person preferably) > - Most style issues should be caught by automated tools I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically. Better yet, I've made a `npm run fix` command that will fix 99% of them instantly. If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)

the_gipsy 5y ago

> Not sure why we have fallen so much in love with the idea. Because organisations obsess about risk minimisation.

cbsmith 5y ago

That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? To me code reviews can catch style and functional problems in code that get by your automation systems, but the primary impact of a code review is on the team, not the code. It imparts not just an understanding and ownership of the code, but also establishes/negotiates how the team works together.

miltondts 5y ago
> That's interesting to consider a code walkthrough as different than a code review. How would you describe the differences? The code review process I'm familiar with is: open a PR, wait for a couple of reviewers to comment on the work. Then normally what happens is: - The comments are trivial, so I implement the changes (these are almost always things that could be automated, but for one reason or another haven't been). - The comments need to be discussed/clarified, so I need to call the person. The code by itself doesn't have all the context, all the things that were tried etc. A code walkthrough is a session with other coders, where it is explained how and why we arrived at the solution, what considerations were made and so on. Walkthrough sessions provide much faster feedback and all improvements can be discussed immediately with much greater clarity.
- cbsmith 5y ago
  Got it. So for my semantics, your review process for PR's sucks, lacking engagement, and you have to have meetings to get it done right.

beebmam 5y ago

This is a deeply unpopular opinion, but I find myself aligning with this more every day. I think code reviews are a fundamentally broken process. Design reviews, before any (non-prototype) code even gets written, are vastly more important than code reviews in my opinion.

01100011 5y ago

Upvoted because these are all great points that I agree with but code review is still worth the trouble in many situations. On my team it helps us keep a consistent style and forces more people on the team to stay up to date with changes. Code quality is improved because of our review process. Pair programming would accomplish some of that, but not all team members would have a voice. Further, reviews are asynchronous. Sure, reviews are a time sink. No one likes them. We are still better off having done them.

valenterry 5y ago

I agree. Code reviews are still a nice "sanity check" where I don't have to understand in full detail what happens and I don't criticize style problems and minor issues. However, I check if the change fits into the bigger picture (in terms of architecture), that the intention of the changes is correct and that tests are added if needed. Hence, a combination of intense pair programming and knowledge sharing sessions in combination with quick & lightweight PR reviews as well as regular refactorings has worked best in my experience.

TheCoelacanth 5y ago

None of those are the type of things that code reviews should be focused on catching. They should be focused on things like: - Does the code make sense to a human? - Is this the best way to achieve the objective? - Is the thing that the code is trying to do even something that should be done?

rixed 5y ago
Those last two are the most important (and less subjective), and should definitively happen before the code is even written, using design docs.

dragonwriter 5y ago

> Most style issues should be caught by automated tools Some style issues can be, others (“do variables, functions, etc. have meaningful, accurate, relevant names?”) can’t be, until the automated tools are backed by AGI, but at that point “code review” and “automated analysis” become...not so different.

omegalulw 5y ago

> To bring someone up to speed on your chosen style, pair programming is much faster than code reviews Nope. People have biases, and if you are teaching coding practices via pair programming you will just propagate those biases. By having a new team member receive feedback from multiple members, these biases are more likely to average out. For style specifically, it's also wise to get reviews from other teams for new people.

twistedpair 5y ago

> - Most functional issues should be caught by tests (written by another person preferably) > - Most style issues should be caught by automated tools I do a lot of Typescript coding. Using Prettier/eslint rules, a host of (e.g. hundreds) of common stylistic and functional issues are caught in CI/CD automatically. Better yet, I've made a `npm run fix` command that will fix 99% of them instantly. If your tooling can automatically bring the code into compliance, then folks won't push back and will be highly likely to comply :)