3 ms·

I am always kind of a stickler about code reviews. I once had a manager tell me that I should leave more to QA with an offhand comment along the lines of "what

by donatj 1y ago

I am always kind of a stickler about code reviews. I once had a manager tell me that I should leave more to QA with an offhand comment along the lines of "what is the worst that could happen" to which I replied without missing a beat "We all lose our jobs. We are always one bad line of code away from losing our jobs" The number of times I have caught junior or even experienced devs writing potential PII leaks is absolutely wild. It's just crazy easy in most systems to open yourself up to potential legal issues.

canucker2016 1y ago
...And if there's no one around to review the code? The website makes it seem like it's a one person shop.
- alias_neo 1y ago
  When I work on my own code, at home, with no-one to assist or review, I write tests, and open a PR anyway, and review it myself, sometimes the next day with fresh eyes, or even 10 minutes later after a quick walk in and out of the room and a glass of water. If you're not confident you can review a piece of code you wrote and spot a potentially disastrous bug like the one in OP, write more tests.
  - zarzavat 1y ago
    Humans are very good at not spotting their own mistakes, that's why writers have editors.
    - rs186 1y ago
      These days you can dump your code/diff to ChatGPT and ask it to criticize. Often most of it is junk -- irrelevant or incorrect suggestions. But the 1 in 10 suggestions is actually useful and correct, which can be immensely helpful.
ValdikSS 1y ago
Yep, what's in most other jobs is a criminal offense, the most serious issue the individual developer could face is just to lose the job.
- monkeyelite 1y ago
  If you demand accountability you need to grant authority.
monkeyelite 1y ago
Code reviews kill velocity - introduce context switching, and are make work, it feels like you’re doing something to make a PR etc but your not. The context it makes the most sense is accepting code from strangers in a low trust environment. The alternative to trying to prevent mistakes is making it easy to find and correct them. Run CI on code after it’s been merged and send out emails if it’s failed. At the end of a day produce a summary of changes and review them asynchronously. Use QA, test environments, etc.
- Capricorn2481 1y ago
  > Code reviews kill velocity - introduce context switching, and are make work This is the same point three times, and I don't agree with it. This is like saying tests kill velocity, there's nothing high velocity about introducing bugs to your code base. Everything introduces context switching, there's nothing special about code reviews that makes it worse than answering emails, but I'm not going to ignore an important email because of "context switching." Everyone makes mistakes, code reviews are a way to catch those. They can also spread out the knowledge of the code base to multiple people. This is really important at small companies. CI is great, but I have yet to see a good CI tool that catches the things I do.
  - monkeyelite 1y ago
    > This is the same point three times No it isn’t. Fake work, synchronization, and context switching are all separate problems. > code reviews are a way to catch those I said you can do reviews - but there is no reason to stop work to do them. Why not require two or three reviews if they are so helpful at finding mistakes? I agree everyone makes mistakes - that’s why I would design a process around fixing mistakes, not screening for perfection. How many times have you gone back to address review comments and introduced a regression because you no longer have the context in your head?
    - loeg 1y ago
      > Why not require two or three reviews if they are so helpful at finding mistakes? Diminishing returns, of course. I have worked places where two reviews were required and it was not especially more burdensome than one, though. I catch so many major errors in code review ~every day that it's bizarre to me that someone is advocating for zero code review.

by donatj 1y ago

canucker2016 1y ago

...And if there's no one around to review the code? The website makes it seem like it's a one person shop.

alias_neo 1y ago
When I work on my own code, at home, with no-one to assist or review, I write tests, and open a PR anyway, and review it myself, sometimes the next day with fresh eyes, or even 10 minutes later after a quick walk in and out of the room and a glass of water. If you're not confident you can review a piece of code you wrote and spot a potentially disastrous bug like the one in OP, write more tests.
- zarzavat 1y ago
  Humans are very good at not spotting their own mistakes, that's why writers have editors.
  - rs186 1y ago
    These days you can dump your code/diff to ChatGPT and ask it to criticize. Often most of it is junk -- irrelevant or incorrect suggestions. But the 1 in 10 suggestions is actually useful and correct, which can be immensely helpful.

ValdikSS 1y ago

Yep, what's in most other jobs is a criminal offense, the most serious issue the individual developer could face is just to lose the job.

monkeyelite 1y ago
If you demand accountability you need to grant authority.

monkeyelite 1y ago

Code reviews kill velocity - introduce context switching, and are make work, it feels like you’re doing something to make a PR etc but your not. The context it makes the most sense is accepting code from strangers in a low trust environment. The alternative to trying to prevent mistakes is making it easy to find and correct them. Run CI on code after it’s been merged and send out emails if it’s failed. At the end of a day produce a summary of changes and review them asynchronously. Use QA, test environments, etc.

Capricorn2481 1y ago
> Code reviews kill velocity - introduce context switching, and are make work This is the same point three times, and I don't agree with it. This is like saying tests kill velocity, there's nothing high velocity about introducing bugs to your code base. Everything introduces context switching, there's nothing special about code reviews that makes it worse than answering emails, but I'm not going to ignore an important email because of "context switching." Everyone makes mistakes, code reviews are a way to catch those. They can also spread out the knowledge of the code base to multiple people. This is really important at small companies. CI is great, but I have yet to see a good CI tool that catches the things I do.
- monkeyelite 1y ago
  > This is the same point three times No it isn’t. Fake work, synchronization, and context switching are all separate problems. > code reviews are a way to catch those I said you can do reviews - but there is no reason to stop work to do them. Why not require two or three reviews if they are so helpful at finding mistakes? I agree everyone makes mistakes - that’s why I would design a process around fixing mistakes, not screening for perfection. How many times have you gone back to address review comments and introduced a regression because you no longer have the context in your head?
  - loeg 1y ago
    > Why not require two or three reviews if they are so helpful at finding mistakes? Diminishing returns, of course. I have worked places where two reviews were required and it was not especially more burdensome than one, though. I catch so many major errors in code review ~every day that it's bizarre to me that someone is advocating for zero code review.