4 ms·

This might be off-Topic but is there any good reason not to do code reviews? All the arguments against code reviews I've read so far are either about fast compl

This might be off-Topic but is there any good reason not to do code reviews? All the arguments against code reviews I've read so far are either about fast completion times on features and staying "agile" or about developers not wanting to bother to do "non-coding" time.

sethammons 3y ago
I advocate for skipping code reviews for code that will not go to production. If it goes to production, it gets reviewed.
- whstl 3y ago
  So, testing, documentation and development-time tooling? Or do you also consider that as "going into production"? Asking because I could interpret your answer both ways, and both are reasonable.
hliyan 3y ago
Skipping code reviews is fundamentally non-agile. It's faster to fix code at review time than post deployment. Joel Spolsky said something similar with respect to technical design too.
oxfordmale 3y ago
Code reviews are often the biggest waste of time in most companies. Either there is a culture of "sledgehammer approve", or a culture of nitpicking. A lot of the feedback provided during code reviews can be provided by coding tools.
ssrc 3y ago
Devil's advocate: PRs are unnecessary and insufficient (this is the article's introduction). For each claimed benefit there's another practice that is better suited, usually automatic, and should have been already implemented. For example, for catching regressions, a reasonably complete test suite. For formatting issues, IDEs and autoformat on commit. For latent errors, static analyzers, linters, run-time instrumentation, etc. Things that cannot be automated, like understanding of the problem, design tradeoffs, etc. should have been discussed with the team before the PR, with or without pair programming. Now, going back to reality, if all of the above is working, the PR should be a breeze and there wouldn't be much debate about them.
- thumbuddy 3y ago
  This is increasingly closer to reality. Some shops though... Whew code review is more of a weapon then anything else.
  - Attummm 3y ago
    Good code reviews are essential for finding bugs, especially those that are hard to spot. They also ensure that the conceptual integrity of the codebase remains intact. Moreover, they allow a developer who is less familiar with the codebase to propose a solution, even if they might not fully grasp the entire context of the project or codebase. This is a significant advantage. However, unfortunately, code reviews can devolve into hazing or become a means to establish power dynamics that shouldn't exist. We should never assume the reviewer knows everything. A productive code review is more of a collaboration rather than a teacher-student relationship. How can we automate issues like using a task UUID as a device UUID within the code? Or addressing an n + 1 problem when another API with batch functionality is available? We can't automate those(yet)
    - thumbuddy 3y ago
      I agree with you. I would argue that most if not all team dysfunction arises during reviews. Ive worked on places where senior engineers will allow bugs like you've mentioned through to production so they can catch the "hard off by one problem" to get the + and stick the - on someone else. Even serious bugs. Yep leads protecting themselves from ambitious seniors and seniors protecting themselves from juniors who shouldn't be juniors. Vindictive practices, gas lighting non-technical management, etc. So with any decent system, the weakest link is the people using it. If you have adversarial people on your team, and in my experience, you probably do, sometimes the system surrounding code review is far more important. IE reviewers share the blame(better yet there is no blame shit just gets fixed), and hard rules around being an asshole need to be enforced. I realize you are describing a healthy work environment. Unfortunately, I've yet to find one. I almost made one once, but a wave of clandestine hires destroyed it and it was time to move on. Pissed off the wrong narcissist...
- 3y ago
rr808 3y ago
We have lots of config in git, which requires code reviews. 90% of PRs are trivial changes. its especially annoying if you're working on your own. Also as a senior I often have to get managers or green newbies to approve, neither which can add any useful feedback.
- Attummm 3y ago
  From the description, it appears the issue is more organizational than related to code reviews. Similar to how 'security theater' addresses perceived threats rather than real vulnerabilities. It takes two parties atleast for productive code review
- whstl 3y ago
  For those cases I just bypass approval. You should try asking for this permission. If you can’t get it, then you have a problem.
darkclouds 3y ago
Code. Test. Optimise. Thats why fuzzers are important for testing, they brute force stupidity so the users dont have to. The only (minor) advantage I see for paired programming is when the coder is out of ideas for some code at the first stage. Some languages are self documenting which eliminates the non coding time. Me personally, I just hate going around in circles (loops), its not elegant code, but it has to be done.
Attummm 3y ago
If we were to reverse the premise to: "How can we create an unmaintainable codebase riddled with bugs?" Then we'd have a good reason not to conduct code reviews
crabbone 3y ago
Those reviewing being less knowledgeable but more willing to change the code being reviewed to be actually worse than original? Happens more than you would've thought, but maybe still not enough to justify the abandonment of the practice... So, in sum, I don't think it's ever worth it to skip the review process.

sethammons 3y ago

I advocate for skipping code reviews for code that will not go to production. If it goes to production, it gets reviewed.

whstl 3y ago
So, testing, documentation and development-time tooling? Or do you also consider that as "going into production"? Asking because I could interpret your answer both ways, and both are reasonable.

hliyan 3y ago

Skipping code reviews is fundamentally non-agile. It's faster to fix code at review time than post deployment. Joel Spolsky said something similar with respect to technical design too.

oxfordmale 3y ago

Code reviews are often the biggest waste of time in most companies. Either there is a culture of "sledgehammer approve", or a culture of nitpicking. A lot of the feedback provided during code reviews can be provided by coding tools.

ssrc 3y ago

Devil's advocate: PRs are unnecessary and insufficient (this is the article's introduction). For each claimed benefit there's another practice that is better suited, usually automatic, and should have been already implemented. For example, for catching regressions, a reasonably complete test suite. For formatting issues, IDEs and autoformat on commit. For latent errors, static analyzers, linters, run-time instrumentation, etc. Things that cannot be automated, like understanding of the problem, design tradeoffs, etc. should have been discussed with the team before the PR, with or without pair programming. Now, going back to reality, if all of the above is working, the PR should be a breeze and there wouldn't be much debate about them.

thumbuddy 3y ago
This is increasingly closer to reality. Some shops though... Whew code review is more of a weapon then anything else.
- Attummm 3y ago
  Good code reviews are essential for finding bugs, especially those that are hard to spot. They also ensure that the conceptual integrity of the codebase remains intact. Moreover, they allow a developer who is less familiar with the codebase to propose a solution, even if they might not fully grasp the entire context of the project or codebase. This is a significant advantage. However, unfortunately, code reviews can devolve into hazing or become a means to establish power dynamics that shouldn't exist. We should never assume the reviewer knows everything. A productive code review is more of a collaboration rather than a teacher-student relationship. How can we automate issues like using a task UUID as a device UUID within the code? Or addressing an n + 1 problem when another API with batch functionality is available? We can't automate those(yet)
  - thumbuddy 3y ago
    I agree with you. I would argue that most if not all team dysfunction arises during reviews. Ive worked on places where senior engineers will allow bugs like you've mentioned through to production so they can catch the "hard off by one problem" to get the + and stick the - on someone else. Even serious bugs. Yep leads protecting themselves from ambitious seniors and seniors protecting themselves from juniors who shouldn't be juniors. Vindictive practices, gas lighting non-technical management, etc. So with any decent system, the weakest link is the people using it. If you have adversarial people on your team, and in my experience, you probably do, sometimes the system surrounding code review is far more important. IE reviewers share the blame(better yet there is no blame shit just gets fixed), and hard rules around being an asshole need to be enforced. I realize you are describing a healthy work environment. Unfortunately, I've yet to find one. I almost made one once, but a wave of clandestine hires destroyed it and it was time to move on. Pissed off the wrong narcissist...
3y ago

rr808 3y ago

We have lots of config in git, which requires code reviews. 90% of PRs are trivial changes. its especially annoying if you're working on your own. Also as a senior I often have to get managers or green newbies to approve, neither which can add any useful feedback.

Attummm 3y ago
From the description, it appears the issue is more organizational than related to code reviews. Similar to how 'security theater' addresses perceived threats rather than real vulnerabilities. It takes two parties atleast for productive code review
whstl 3y ago
For those cases I just bypass approval. You should try asking for this permission. If you can’t get it, then you have a problem.

darkclouds 3y ago

Code. Test. Optimise. Thats why fuzzers are important for testing, they brute force stupidity so the users dont have to. The only (minor) advantage I see for paired programming is when the coder is out of ideas for some code at the first stage. Some languages are self documenting which eliminates the non coding time. Me personally, I just hate going around in circles (loops), its not elegant code, but it has to be done.

Attummm 3y ago

If we were to reverse the premise to: "How can we create an unmaintainable codebase riddled with bugs?" Then we'd have a good reason not to conduct code reviews

crabbone 3y ago

Those reviewing being less knowledgeable but more willing to change the code being reviewed to be actually worse than original? Happens more than you would've thought, but maybe still not enough to justify the abandonment of the practice... So, in sum, I don't think it's ever worth it to skip the review process.