4 ms·

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after

I can’t imagine any reason to skip code reviews in any professional development. Someone is paying the team to produce professional code, finding out well after the fact that one section/contributed failed that test is unacceptable. Code reviews shouldn’t be lots of overhead. They shouldn’t devolve into style critiques. They should be the best tool you have for teaching and communicating best practices. Reviewing a more senior devs code can teach a junior a lot, and occasionally catch significant errors.

sharkdesk 9y ago
Actually, someone is paying the team to solve business problems and create revenue.
- Clubber 9y ago
  Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.
  - Anticapitalist 9y ago
    But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?
- valuearb 9y ago
  The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.
  - Clubber 9y ago
    >Bugs going in testing are far more expensive to fix than those found in code review. How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.
    - valuearb 9y ago
      We always do code review before QA. But our QA process is also multilayered, first layer is QA engineer, next layer is first level business acceptance testing, then another higher level business acceptance testing. Gets pretty expensive once the SR marketing person rejects. No i didn’t design this process.
watwut 9y ago
"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew. If you have expectations, tell them in advance. It is just unfair to use code review for that.
- Clubber 9y ago
  That reminds me of a SQL code reviewer at my last company. He'd have a standard that we would follow and finally get stuff passed most of the time. After a few weeks, he would read a new article and completely change his passing criteria without telling anyone, so all of our previously passing submissions would then fail. This happened several times over the course of a year. It was very frustrating and rarely helped the overall quality of the SQL.
- alangpierce 9y ago
  > If you want communicate new best practice, do so. I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started. Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review. Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.
- valuearb 9y ago

sharkdesk 9y ago

Actually, someone is paying the team to solve business problems and create revenue.

Clubber 9y ago
Preferably in the most efficient way possible (which is of course subjective). I believe that after 3-6 months in a particular subsystem, the ROI of code reviews increasingly decreases over time.
- Anticapitalist 9y ago
  But shouldn't the time required to review also drop significantly when the reviewer is familiar with the module?
valuearb 9y ago
The people who pay my team mostly want bugs fixed. Bugs going in testing are far more expensive to fix than those found in code review.
- Clubber 9y ago
  >Bugs going in testing are far more expensive to fix than those found in code review. How so? Particularly if you don't do your code review right after development. That holds true for production vs. QA, but I'm not sure it does for QA vs. code review, unless your QA deployment and ticket bounce back process is overly cumbersome and you don't have time planned for it.
  - valuearb 9y ago
    We always do code review before QA. But our QA process is also multilayered, first layer is QA engineer, next layer is first level business acceptance testing, then another higher level business acceptance testing. Gets pretty expensive once the SR marketing person rejects. No i didn’t design this process.

watwut 9y ago

"They should be the best tool you have for teaching and communicating best practices. " For christ sake, no. If you want communicate new best practice, do so. And if team agrees, we will follow. Don't ambush me with new expectations during code review and don't keep my task hostage in order to push for your new best practice. Same with teaching. Teach proactively instead of pretending juniors can solve it and then rewrite all their decisions in code rewiew. If you have expectations, tell them in advance. It is just unfair to use code review for that.

Clubber 9y ago
That reminds me of a SQL code reviewer at my last company. He'd have a standard that we would follow and finally get stuff passed most of the time. After a few weeks, he would read a new article and completely change his passing criteria without telling anyone, so all of our previously passing submissions would then fail. This happened several times over the course of a year. It was very frustrating and rarely helped the overall quality of the SQL.
alangpierce 9y ago
> If you want communicate new best practice, do so. I think the quote your replying to means existing best practices. IMO, things like "we tend to follow this naming convention" and "here's a useful language feature you may not know about" seem completely fine to say at code review time, and it's not like you can teach someone absolutely everything before they get started. Having a reviewer hold a review hostage is a big cultural red flag, IMO. If there's a real significant disagreement, you shouldn't try to solve it via code review comments; you should move to in-person communication if possible and maybe pull other people in to take a look. I also think that in most situations, reviewers should bias toward suggestions and ideas rather than demands ("strong opinions, weakly held"), and authors should feel comfortable pushing back or proposing to merge the commit as-is and leave some change for a follow-up review. Saying code reviews are "the best tool" for teaching and communicating best practices is maybe a stretch; I find things like pair programming to be much better for stuff like that. But I still think code reviews can be a great mechanism for teaching code-level details like code style.
valuearb 9y ago