5 ms·

You know what I hate more than code review? Shipping bugs. The author of this piece admits that they have to change the code they write in response to comment

You know what I hate more than code review? Shipping bugs. The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Similarly, when acting as the code reviewer, the author is giving feedback to others (rather than just rubber-stamping the PR), and presumably that feedback is implemented. So code review seems to be doing its job here. It's producing better code. Ultimately I think the problem is revealed at the end, where the author says they feel like code review is optional. If you think it's optional, then the delays inherent in review are going to feel like artificial slowdowns. The better way to think about it is that code review simply isn't optional. Yes it can be a pain, and there's a lot of friction in the process, and our tooling kinda sucks for it. But it's a necessary step. I can't even begin to count how many bugs my team has avoided shipping due to code review. And I always get really annoyed when I run across a bug that should have been caught in code review but wasn't for whatever reason (perhaps a more junior coder did the review and therefore missed stuff that a more senior coder would have caught). Code review stops bugs before they happen. And the best bug is the one that was never shipped to customers.

onion2k 8y ago
Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Only if the author cares enough about the code. I've implemented things I know are bad in the past because the politics of pushing back aren't worthwhile. Especially if you work in an environment where your review is driven by the opinion of your colleagues (eg stack ranking). "Code reviews are painful" are often a sign of bad team dynamics or a culture of politically motivated decisions.
lowercased 8y ago
much like 'testing', the code will get reviewed at some point - either during a crisis, or before it goes out.
falsedan 8y ago
Code review doesn’t catch bugs. Sure, sometimes an experienced dev can spot a bounds error or race condition, but CRS are for knowledge sharing and style adherence.
- mycelium 8y ago
  Completely disagree. I catch bugs, and have my bugs caught, at least a few times a month. Beyond the "this will likely deadlock without a timeout", "this does not clean up after itself on exception", "if the cache is not hot, this operation you assume is 1 sec will take 1 min" type thing — one of the strongest points of code review is having a conversation about "this code is hard to understand" and turning code that's correct if you think about it really hard into obviously correct code. It takes a reasonably strong reviewer to get there though, and that's not always possible.
- zimpenfish 8y ago
  > Code review doesn’t catch bugs. It can but it's definitely not a 100% surety (as some people seem to think.)
- arnvald 8y ago
  I review code everyday and it's my main way of finding bugs (besides automated tests). It saves us a lot of time that I can detect issues before asking testers to run the application and go through test scenarios.
Jyaif 8y ago
> Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Sometimes the suggestions add very little / nothing. Push-backing (i.e. adding a back and forth) would be a waste of time so you just do whatever the code reviewer says. > So code review seems to be doing its job here. It's producing better code. As with a lot of things, balance is needed. Better code comes with a cost (time), and that time may have been better spent somewhere else.
Vinnl 8y ago
> where the author says they feel like code review is optional. If you think it's optional I think these are two different things. The author says they don't enjoy code review, and that that probably is because it feels optional. From the post, I gathered that that's not what the author thinks - the first few paragraphs emphasises that they think it is an important part of the work. But apparently, that knowledge isn't enough to change the experience of reviewing.
bryanrasmussen 8y ago
so another story here: code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug) Got comment - you need to change em to be rem. I can't do that because there is em all over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place. Those are the rules fix them. since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes. fixed overflow bug other way. no code reuse ever in that part of codebase.
- RandomInteger4 8y ago
  My CSS-foo is fairly weak; what problems would em cause such that they disallow it in the code base?
  - bryanrasmussen 8y ago
    em is relative to the font size of the nearest parent thus you can have situations where your font size of an element node is radically different due to the parent it is placed in. This can of course be powerful if used correctly, but it takes someone really good to use it correctly and perhaps it shouldn't be done in big teams because big teams imply some people not very good in some particular discipline. on edit: adding this link https://j.eremy.net/confused-about-rem-and-em/ https://j.eremy.net/confused-about-rem-and-em/
    - pm24601 8y ago
      Thanks! I never understood this difference between em, rem, px. I learned something very new and I have been doing web development (backend mostly) for a long time.
  - dredmorbius 8y ago
kace91 8y ago
> The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). There's a potential problem with that idea: The assumption that, if several programmers involved offer different solutions, the best solution will prevail. I think in many cases the accepted solution ends up being the one pushed by the person that holds more power, the one who can communicate better, or the person that 'negotiates' more aggressively. I'm not saying that to argue against code reviews - It's just a friendly reminder that although as technical people we hope to achieve some objective level of quality, we can't escape office politics, human bias and general subjectivity.
- wool_gather 8y ago
  You're right. I'd say it's an aspect of professionalism to get better at this as a reviewer. One of the things you need to try to do when you read somebody else's pull request is to put aside your ego and focus on the code.
lostcoder 8y ago
I'm fortunate enough at my company to not have to submit to the code review bureaucracy - and I can justify it by having metrics which indicate that my code ships with fewer bugs than the code of other teams which do go through code reviews. I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured by achievement of business goals and revenue. For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality. So yes, code review IS optional if you can justify it.
- mfukar 8y ago
  I like how you emphasise the "IS" instead of the "if".
  - betageek 8y ago
    For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality. Sounds like you have bigger problems than code review!
- arnvald 8y ago
  What's the justification here though? Saying "my code is better than other teams' even without review" does not prove that reviews are not helpful - maybe if you get your code reviewed you'll ship even fewer bugs, and if other teams don't review they'll ship even more bugs.
  - addicted 8y ago
    Further, eliminating bugs caused by any individual code commit is not really the goal behind code reviews in the first place. Finding bugs that the user notices on immediate use is an indictment of the QA process (whether manual or automatic). Code reviews help maintain sanity in the code base, and are useful for sharing knowledge (both in terms of quality of code, and ensuring that there are at least 2 people who have some idea behind the code written). It provides an opportunity to have it read by someone who doesn’t share the same priors as you and helps you identify sections of code that may be confusing for another developer (including future you). If Code review does lead to the identification of bugs that’s just a bonus. There’s a reason Code review does not have a “run the code and test it” component to it and is entirely focused on code, and not whether it runs correctly.
oldboyFX 8y ago
> The better way to think about it is that code review simply isn't optional. Of course it's optional. Tests are optional too. There are many projects and situations where trading code quality for development speed makes sense. Especially when you're building MVP's, experimenting, or trying to validate market demand. It's also often not worth it on small to medium sized projects where only one person is working on the codebase (think 3-person project: designer, front-end dev, back-end dev). Larger projects with multiple people working on the same codebase are a completely different story. In those cases code reviews are valuable and highly recommended but still optional. The only thing that isn't optional is delivering a working product. Best practices == great, dogma == not great.
Eleopteryx 8y ago
To me, code review is a combination of two things I would think we would try to avoid: 1. Testing implementation over behavior 2. Using a human being to test in a non-automated way Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is an efficient means to catch that if user is in a state of X and tries to do Y to Z that the app blows up. That becomes most apparent when that code is running in the full context of the application, not when viewed as a git diff. A bug is so often a perfect storm of seemingly innocuous but conflicting/insufficient behavior than it is some block of code that is clearly broken at a glance. There is something to be said about stylistic and architectural issues and code reviews do give an opportunity for asking questions and driving "better" code. But this is actually just another reason why I don't like code review. "Better" is subjective and there's always another level of "better" to be reached. If I held them to my own personal standards sometimes I'd be asking for a rewrite. So what do I do, suggest that they improve their code, but not to the full extent that I feel it could be improved? I hate code review.
- eridius 8y ago
  There are so many bugs that I've caught in code review that would not have been caught by automated or manual testing. Two big reasons for that: 1. Automated testing is still written by a human. A human who doesn't understand the edge cases in their code isn't going to understand how to write tests for those edge cases either. And "have someone else write the tests" won't work either, because frequently these edge cases are a result of the code implementation, not inherent in the problem, and so without a thorough understanding of the code you wouldn't even know the edge cases is there to be tested. Same goes for manual testing too, the manual testcases are still constructed by a human. 2. Race conditions, or rare conditions that most people won't hit. These are rather unlikely to be uncovered by testing (whether automated or manual). Race conditions in general are hard to test, and rare conditions are, well, rare. There's also just cases like unexpected interaction paths that can produce improperly-handled states in the code, or bugs that don't result in an obvious bug to the observer but are still incorrect (the obvious example here is memory leaks, but this could also just be leaving the app in a bad state such that subsequent actions would fail, but you don't catch those because your test is done at this point).
ericmcer 8y ago
Wouldn’t bugs be caught during dev testing, or QA testing or automated testing or while running on a staging server? It seems nuts to rely on a developer looking at a code diff to catch bugs in an app, it happens sometimes but I generally think review is more about quality.

onion2k 8y ago

Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Only if the author cares enough about the code. I've implemented things I know are bad in the past because the politics of pushing back aren't worthwhile. Especially if you work in an environment where your review is driven by the opinion of your colleagues (eg stack ranking). "Code reviews are painful" are often a sign of bad team dynamics or a culture of politically motivated decisions.

lowercased 8y ago

much like 'testing', the code will get reviewed at some point - either during a crisis, or before it goes out.

falsedan 8y ago

Code review doesn’t catch bugs. Sure, sometimes an experienced dev can spot a bounds error or race condition, but CRS are for knowledge sharing and style adherence.

mycelium 8y ago
Completely disagree. I catch bugs, and have my bugs caught, at least a few times a month. Beyond the "this will likely deadlock without a timeout", "this does not clean up after itself on exception", "if the cache is not hot, this operation you assume is 1 sec will take 1 min" type thing — one of the strongest points of code review is having a conversation about "this code is hard to understand" and turning code that's correct if you think about it really hard into obviously correct code. It takes a reasonably strong reviewer to get there though, and that's not always possible.
zimpenfish 8y ago
> Code review doesn’t catch bugs. It can but it's definitely not a 100% surety (as some people seem to think.)
arnvald 8y ago
I review code everyday and it's my main way of finding bugs (besides automated tests). It saves us a lot of time that I can detect issues before asking testers to run the application and go through test scenarios.

Jyaif 8y ago

> Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). Sometimes the suggestions add very little / nothing. Push-backing (i.e. adding a back and forth) would be a waste of time so you just do whatever the code reviewer says. > So code review seems to be doing its job here. It's producing better code. As with a lot of things, balance is needed. Better code comes with a cost (time), and that time may have been better spent somewhere else.

Vinnl 8y ago

> where the author says they feel like code review is optional. If you think it's optional I think these are two different things. The author says they don't enjoy code review, and that that probably is because it feels optional. From the post, I gathered that that's not what the author thinks - the first few paragraphs emphasises that they think it is an important part of the work. But apparently, that knowledge isn't enough to change the experience of reviewing.

bryanrasmussen 8y ago

so another story here: code review mandatory at last place I worked, the place had a policy of any code you push in that does not match the guidelines must be changed. Guidelines was that no em allowed, must use rem. Old part of codebase assigned to me, I found some things in CSS I improved (reuse of code, a small overflow bug) Got comment - you need to change em to be rem. I can't do that because there is em all over the place and this can have side effects. The diff shows it as my code but if you look you can see it is actually the old code just moved to right place. Those are the rules fix them. since this code was in a big release of stuff and none of our stuff was allowed through unless I changed one em to rem even though I could not be sure there wouldn't be problems I went ahead and rolled back my changes. fixed overflow bug other way. no code reuse ever in that part of codebase.

RandomInteger4 8y ago
My CSS-foo is fairly weak; what problems would em cause such that they disallow it in the code base?
- bryanrasmussen 8y ago
  em is relative to the font size of the nearest parent thus you can have situations where your font size of an element node is radically different due to the parent it is placed in. This can of course be powerful if used correctly, but it takes someone really good to use it correctly and perhaps it shouldn't be done in big teams because big teams imply some people not very good in some particular discipline. on edit: adding this link https://j.eremy.net/confused-about-rem-and-em/ https://j.eremy.net/confused-about-rem-and-em/
  - pm24601 8y ago
    Thanks! I never understood this difference between em, rem, px. I learned something very new and I have been doing web development (backend mostly) for a long time.
- dredmorbius 8y ago

kace91 8y ago

> The author of this piece admits that they have to change the code they write in response to comments. This means the code review is flagging areas of improvement. Surely the resulting code is better than the original code (otherwise I'd expect the author to push back on the comments instead of implementing bad suggestion). There's a potential problem with that idea: The assumption that, if several programmers involved offer different solutions, the best solution will prevail. I think in many cases the accepted solution ends up being the one pushed by the person that holds more power, the one who can communicate better, or the person that 'negotiates' more aggressively. I'm not saying that to argue against code reviews - It's just a friendly reminder that although as technical people we hope to achieve some objective level of quality, we can't escape office politics, human bias and general subjectivity.

wool_gather 8y ago
You're right. I'd say it's an aspect of professionalism to get better at this as a reviewer. One of the things you need to try to do when you read somebody else's pull request is to put aside your ego and focus on the code.

lostcoder 8y ago

I'm fortunate enough at my company to not have to submit to the code review bureaucracy - and I can justify it by having metrics which indicate that my code ships with fewer bugs than the code of other teams which do go through code reviews. I think everyone should have some level of freedom to work in a manner which works best for them, rather than forcing everyone into the same process. Success should be measured by achievement of business goals and revenue. For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality. So yes, code review IS optional if you can justify it.

mfukar 8y ago
I like how you emphasise the "IS" instead of the "if".
- betageek 8y ago
  For every anecdote you have about code reviews finding bugs, I have my own anecdote about people writing code going through a thorough code review process and the code crashes the first time an end user tries to use the functionality. Sounds like you have bigger problems than code review!
arnvald 8y ago
What's the justification here though? Saying "my code is better than other teams' even without review" does not prove that reviews are not helpful - maybe if you get your code reviewed you'll ship even fewer bugs, and if other teams don't review they'll ship even more bugs.
- addicted 8y ago
  Further, eliminating bugs caused by any individual code commit is not really the goal behind code reviews in the first place. Finding bugs that the user notices on immediate use is an indictment of the QA process (whether manual or automatic). Code reviews help maintain sanity in the code base, and are useful for sharing knowledge (both in terms of quality of code, and ensuring that there are at least 2 people who have some idea behind the code written). It provides an opportunity to have it read by someone who doesn’t share the same priors as you and helps you identify sections of code that may be confusing for another developer (including future you). If Code review does lead to the identification of bugs that’s just a bonus. There’s a reason Code review does not have a “run the code and test it” component to it and is entirely focused on code, and not whether it runs correctly.

oldboyFX 8y ago

> The better way to think about it is that code review simply isn't optional. Of course it's optional. Tests are optional too. There are many projects and situations where trading code quality for development speed makes sense. Especially when you're building MVP's, experimenting, or trying to validate market demand. It's also often not worth it on small to medium sized projects where only one person is working on the codebase (think 3-person project: designer, front-end dev, back-end dev). Larger projects with multiple people working on the same codebase are a completely different story. In those cases code reviews are valuable and highly recommended but still optional. The only thing that isn't optional is delivering a working product. Best practices == great, dogma == not great.

Eleopteryx 8y ago

To me, code review is a combination of two things I would think we would try to avoid: 1. Testing implementation over behavior 2. Using a human being to test in a non-automated way Proper test coverage and human QA to me is the most effective approach to not shipping bugs, because it really doesn't matter to the customer what the code does or doesn't look like if it runs. And I just don't believe that code review is an efficient means to catch that if user is in a state of X and tries to do Y to Z that the app blows up. That becomes most apparent when that code is running in the full context of the application, not when viewed as a git diff. A bug is so often a perfect storm of seemingly innocuous but conflicting/insufficient behavior than it is some block of code that is clearly broken at a glance. There is something to be said about stylistic and architectural issues and code reviews do give an opportunity for asking questions and driving "better" code. But this is actually just another reason why I don't like code review. "Better" is subjective and there's always another level of "better" to be reached. If I held them to my own personal standards sometimes I'd be asking for a rewrite. So what do I do, suggest that they improve their code, but not to the full extent that I feel it could be improved? I hate code review.

eridius 8y ago
There are so many bugs that I've caught in code review that would not have been caught by automated or manual testing. Two big reasons for that: 1. Automated testing is still written by a human. A human who doesn't understand the edge cases in their code isn't going to understand how to write tests for those edge cases either. And "have someone else write the tests" won't work either, because frequently these edge cases are a result of the code implementation, not inherent in the problem, and so without a thorough understanding of the code you wouldn't even know the edge cases is there to be tested. Same goes for manual testing too, the manual testcases are still constructed by a human. 2. Race conditions, or rare conditions that most people won't hit. These are rather unlikely to be uncovered by testing (whether automated or manual). Race conditions in general are hard to test, and rare conditions are, well, rare. There's also just cases like unexpected interaction paths that can produce improperly-handled states in the code, or bugs that don't result in an obvious bug to the observer but are still incorrect (the obvious example here is memory leaks, but this could also just be leaving the app in a bad state such that subsequent actions would fail, but you don't catch those because your test is done at this point).

ericmcer 8y ago

Wouldn’t bugs be caught during dev testing, or QA testing or automated testing or while running on a staging server? It seems nuts to rely on a developer looking at a code diff to catch bugs in an app, it happens sometimes but I generally think review is more about quality.