Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

No code reviews by default

raycast.com

Post reply on HN
Print view View original

221–230 of 315 posts

Re: No code reviews by default

#221

Post by tombert »

dsjoerg wrote: post #197
tombert wrote: post #131

Earlier quoted context omitted.

There actually were unit tests for it, there was just an edge case I didn’t think to test that caused things to crash, which caused a cascading error.

Sure. Seems like a classic "seven whys" kind of situation... coming down hard on the person who wrote the bug is counterproductive in a way that the whole tech industry understands, except for your former bosses at Apple.

To be clear, I’m not absolving myself on this. The bug was still my fault at the end of the day, I just feel like my managers handled it poorly.

Re: No code reviews by default

#222

Post by hsn915 »

alecbz wrote: post #27
tus666 wrote: post #7

He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…

Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out. That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the pr…

Books are published once, not through iterations.

The equivalent in book writing would be that you are not allowed to write more than one page at a time, and someone must approve it before you start working on the next page.

Re: No code reviews by default

#223

Post by hsn915 »

ljm wrote: post #14
tus666 wrote: post #7

He mentions "trust" a lot. Is that why we do code reviews, because we don't trust each other? I find this attitude problematic. We do code reviews because humans make mistakes. Requirements can be misinterpreted. Different work in progress can be in conflict with each other. Reviews are a good way to learn from each other and keep abreast of what work is occurring outside your own bubble. Not doing code reviews becau…

If something hit production and caused a major fuck-up because there was no peer review process, then in all the places I've worked at the first action item in the post-mortem would be "we should introduce peer review." Otherwise someone would ask how we could ensure it wouldn't happen again, and no one would be able to say "because we trust them," and leave it at that. It would sound more like "I trust you will neve…

This strikes me as weird excuse for code reviews.

To spot quality problems before going to production .. what you need is testing.

Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits.

You need a separate QA team that handles testing - manual and automated.

Re: No code reviews by default

#224

Post by lewisl9029 »

munchbunny wrote: post #209
lewisl9029 wrote: post #144

Earlier quoted context omitted.

Yeah exactly, code review isn't a good place to catch actual bugs. Humans are terrible at catching bugs consistently... Humans probably introduced those bugs in the first place. It's also an exceedingly poor use of human time, which costs a heck of a lot more than machine time. We really should be relying on automated systems to catch bugs for the most part, and leave code review for what it's good for: making sure a…

How do you systematically (and proactively) ensure your testing is thorough? With testing you often run into unknown unknown cases. I’ve always code reviewed/gotten code reviewed on both the implementation and the tests, with completeness as the main quality you’re reviewing test code for. If that’s how we do it, we’re right back at code reviews.

That's definitely an unsolved (unsolvable?) problem. The best we can really do is start with a common sense set of initial tests based on the user observable behaviors we want from the system and then add more tests organically as we learn more about the system's edge cases through incidents, bugs and regressions.

I'm not saying code review can't find any bugs (in practice bugs are found in code review all the time), I'm just saying that if we do find a bug through code review, it means we got lucky, and we shouldn't count on getting lucky again, so better add a test/linter rule for the next time when we might not get so lucky. That's just one more way to grow your automated systems organically.

Re: No code reviews by default

#225

Post by kevingadd »

hsn915 wrote: post #223
ljm wrote: post #14

Earlier quoted context omitted.

If something hit production and caused a major fuck-up because there was no peer review process, then in all the places I've worked at the first action item in the post-mortem would be "we should introduce peer review." Otherwise someone would ask how we could ensure it wouldn't happen again, and no one would be able to say "because we trust them," and leave it at that. It would sound more like "I trust you will neve…

This strikes me as weird excuse for code reviews. To spot quality problems before going to production .. what you need is testing. Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits. You need a separate QA team that handles testing - manual and automated.

If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.

Re: No code reviews by default

#226

Post by kgeist »

It probably makes sense when all the developers are experienced seniors with deep knowledge of the codebase, but often it's not the case. I don't think it scales beyond a small team of senior founders who only start making their first MVP. According to Crunchbase, they were founded 2 years ago and have below 10 employees.

At our company, code reviews help:

1) onboard new devs and teach them about the codebase, catch silly rookie mistakes early on

2) code reviews encourage to write readable code, what I write is readable to me, but can be incomperehensible to others, and code reviews help figure it out

3) seniors make mistakes, too, and it's nice to have another line of defense (some mistakes can be ticking bombs and not catchable in testing immediately)

In my opinion, as a developer, you shouldn't even trust yourself, let alone others, to deliver good product (especially if it's mission-critical software with a large userbase), because errare humanum est.

Re: No code reviews by default

#227

Post by pc86 »

kevingadd wrote: post #225
hsn915 wrote: post #223

Earlier quoted context omitted.

This strikes me as weird excuse for code reviews. To spot quality problems before going to production .. what you need is testing. Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits. You need a separate QA team that handles testing - manual and automated.

If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.

Major errors are easy to spot and will likely take QA minutes to find. They're also probably not going to do a full test of the entire system with every change unless you've got a very small product.

One thing I've seen that I like a lot is individual changes creating individual front ends. You test the one thing in that change, and it's done. This tends to cause a lot of problems with CORS, but then what doesn't?

Agreed with you though that good code reviews (whatever that means) obviates most of this.

Re: No code reviews by default

#228

Post by kqr »

kevingadd wrote: post #225
hsn915 wrote: post #223

Earlier quoted context omitted.

This strikes me as weird excuse for code reviews. To spot quality problems before going to production .. what you need is testing. Not "unit testing" - which is just another pointless process that does not actually provide the purported benefits. You need a separate QA team that handles testing - manual and automated.

If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.

Correct. Any time you need a completely separate team to handle any aspect of the lifecycle of the product you're introducing delays and miscommunication. Besides, a separate team to do a part of the job scales badly: it needs to grow at least linearly with the number of developers, sometimes superlinearly.

You should absolutely have a small QA team, but their job shouldn't be doing the QA -- it should be helping others do their own QA.

Re: No code reviews by default

#229

Post by kqr »

pc86 wrote: post #227
kevingadd wrote: post #225

Earlier quoted context omitted.

If you don't catch major errors until the QA team has time to do a full comprehensive test of the product then those errors are probably sitting in the codebase for days or weeks, and additional changes are being piled on top of them which will make it harder to identify which commit was the problem. Do a code review and you'll catch a bunch of stuff before it even hits QA.

Major errors are easy to spot and will likely take QA minutes to find. They're also probably not going to do a full test of the entire system with every change unless you've got a very small product. One thing I've seen that I like a lot is individual changes creating individual front ends. You test the one thing in that change, and it's done. This tends to cause a lot of problems with CORS, but then what doesn't? Ag…

The actual job is always fast. What takes time is for the job to sit in the queue outside the QA department. The figurative papers moving between desks is always what kills your efficiency, not people doing work slowly.

Re: No code reviews by default

#230

Post by saurik »

kcb wrote: post #90
watwut wrote: post #41

Earlier quoted context omitted.

Ok, but many companies don't use such ridiculous process.

It's not as ridiculous as it sounds as long as the testing and verification happens as part of the PR.

Sure, but then you are just begging the question: if you have a better process for release verification you might not be need any per-commit code review at all.
Post reply on HN