Live data from Hacker News

No code reviews by default

raycast.com

131–140 of 315 posts

Re: No code reviews by default

#131

I think I largely agree with this article. When I worked for Apple, we had a strict "all code needs to be reviewed" policy, which I had no problem with. Then, after being there for about 1.5 years, I make a PR, ask two different people to approve it, which they do, and it gets merged. A week later, the code is released, it looks like my code caused a fairly major bug. My manager's manager and my manager had a meeting…

> test it quickly Yes, in your scenario the lack of testing stands out as the major opportunity for improvement. If a bug is so important that it's mentioned in a performance review, then it's important enough that there should be tests that would have caught it. Automated preferably, or manual if necessary. And everyone involved with the software, from you on up and sideways, should be calling for this testing. It's…

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.

Re: No code reviews by default

#132
Code reviews where some coworker looks at the changes, types "LGTM", and proceeds to approve it do next to nothing to prevent bugs and bad decisions.

Code reviews where some coworker runs the code on their machine and has plenty of time and power to express the ways it could break or it could be improved are useful.

When the former review happens more often than the latter, if often has to do with soft skills rather than hard skills. Some requirements for a good review are honesty, mutual respect, and ability to handle criticism. Other than that, some engineers struggle to understand there may be more than one good approach to a problem.

Ultimately, engineers are not entirely to blame for bad reviews. In my experience it is often a company culture issue. Authoritarian leadership trickles down to middle management, senior engineers, etc. If people don't feel empowered to challenge others code regardless of their seniority or perceived status, weird or downright bad technical decisions will be made.

Re: No code reviews by default

#133
post #62
post #46

Earlier quoted context omitted.

> there’s always that 10% where a second pair of eyes catches a mistake or offers a suggestion that makes the code even better. Testing is pretty awesome process to find bugs.

QA departments are expensive. Execs in their infinite wisdom opted to dump that task on developers who earn 3x as much as QA people. Synergies!

As a dev, I'd much rather do my own testing then have to explain extremely basic shit to the average SDET I've worked with. No, Chuck, the 404 for the favicon on page A did not cause the padding to be off on page B.

Re: No code reviews by default

#134

> Pull requests don't prevent bugs. Is this some anti-vax satire? Of course pull requests (code reviews) reduce bugs. And of course some slip through. It doesn't need to be 100% to be useful.

Yeah, in the past month, I've caught out 2 bugs in a code review. One was a performance degradation due to a function being passed down into a react `useMemo` hook dependency array. The other was that the code was fixing a symptom, but not the cure.

I guess it depends on the quality of the reviewers.

Re: No code reviews by default

#135
I don't trust myself to never, ever introduce a bug. I'd rather that my teammates look it over, because they may have more context on certain areas of the system than I do, or even can just a typo.

I trust my teammates, but if "trust" means "assume they are perfect developers with perfect knowledge of every part of the system who will never make a mistake", sure I don't trust them I guess.

Re: No code reviews by default

#136
post #44
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…

The best code I have seen was on projects without code review. And projects with it were more mess - they were surface consistent but overall hard to comprehend. The deciding factor was ownership and accountability tho - you maintained own code and if you done it crappily, you knew. The code review is related to assumption that everyone can change everything - meaning all in all inconsistent mess. It is also related…

The more problems you have the more policies you add to prevent future problems. I really doubt that a low quality codebase would become higher quality with the addition of code reviews and vice versa.

Re: No code reviews by default

#137
post #14
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…

> For me, it's been hard to go back to my old, cavalier attitude after that.

I’ve heard from my workplace’s software team that this makes hiring from certain sectors particularly difficult because they don’t get this. Too used to move fast and break things and MVP.

Re: No code reviews by default

#138
According to LinkedIn, this company has 12 employees. Once they hit like 20 - 50 engineers and some decide to write code "their way" and some leave, it's going to be a mess to be onboarded there and make sense of code you haven't written yourself.

Re: No code reviews by default

#139
post #44
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…

The best code I have seen was on projects without code review. And projects with it were more mess - they were surface consistent but overall hard to comprehend. The deciding factor was ownership and accountability tho - you maintained own code and if you done it crappily, you knew. The code review is related to assumption that everyone can change everything - meaning all in all inconsistent mess. It is also related…

I prefer a top down where project leads are responsible for code, I.e. they review all code and they are simultaneously responsible for getting things done. It forces balance. I will let juniors make a mess in certain areas and not others. Sometimes you even let bugs in if you know a specific test will fail so people are more careful and don't depend on reviews too much. If you are a bad lead and nitpick every little thing, then you get to explain why your project isn't ready. Honestly getting devs to git add -p and look over their own pr catches much of it.

Re: No code reviews by default

#140
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…

We (at least should) also do code reviews in order to make sure everyone understands the code. On my team, the majority of code review comments are not discussing potential bugs, they're making sure everybody knows how the new thing works, why it was designed that way, implementation tradeoffs, etc. All that discussion is extremely valuable over the long run. For example, it means that nobody has to pay attention to…

> We (at least should) also do code reviews in order to make sure everyone understands the code.

This is the most common reason I see cited for code reviews, but I'm not entirely sure if I buy it, at least not for a lot of teams.

In most cases you are going to have one, maybe two other people looking at a given PR. The positive way of looking at this is that you've just doubled (in some cases tripled) the number of people who are familiar with the code. In reality though, this is still going to be a very small fraction of most engineering organizations. If you have a small team who wholly owns a small codebase then it could be meaningful, but a lot of the time you're still talking about a rounding error from zero people knowing the code- most people in the organization will still be touching the code in the future without any prior knowledge of it, and will have to start understanding it from zero.

Even if you are dealing with a small enough code base, in a small enough team, that the reviewer is adding a substantial margin of familiarity with the codebase to the team, reviewing the code is only going to give an extremely shallow level of understanding. The reviewer isn't going to have all of the context that the original developer had. They won't know all of the things that weren't done or why, the tradeoffs that were made, requirements, business context, etc. Best case scenario, someone wrote that down in a well-known place (comments in the code near the feature, something relevant in a docs directory, hopefully not as an archived jira issue that will never be looked at again). The reviewer probably has their own work to do anyway, and once they approve the PR they'll flush most of what they had out of their short-term memory anyway.

In a lot of ways, this is the same set of reasons that people aren't very likely to find bugs in a code review either. Someone without a lot of context on the problem isn't likely to find many business logic errors, or missed requirements. Since you were busy working on your feature, you probably don't know who else on what other teams might have been doing something relevant to your work, so you wouldn't know to get a review from people who might have something useful to contribute (and they wouldn't know to proactively review your code). One of the reason senior engineering roles often devolve into telling people to talk to one another is to try to address this kind of situation, but that only scales up to a small fraction of the overall code churn at organizations of any size.

For what it's worth, I ask for PRs, and I review my co-workers code. I make comment when I see something that might be useful, and I graciously accept feedback and try to make sensible decisions about when to make changes and when to let the feedback go. I actively search for PRs that might touch sections of the code I've been working in or am very familiar with to try to optimize the quality of the feedback I can give. I'm not really convinced any of this actually helps deliver better code in the moment, but maybe it does some good as a way to help teach my co-workers about things (or to learn about things they can teach me), and anyway it's such an ingrained part of engineering culture that it doesn't seem worth the political capital you'd have to spend to try to change it, and even if you succeeded it would be a prime target for scapegoating if anything went wrong later. It does slow down the development process I think, but people need down time anyway. If waiting on a code review gets people an hour or two of downtime to check their email or read HN between tasks then maybe it's actually worth it for that. The burnout caused by superfluous process and the occasionally large pile-ons and people demanding poorly thought out changes, gatekeeping your ability to deliver business critical work is essentially a cost of doing business that we seem to have collectively accepted in software.

I should also note that I also work on some open source projects, and in that setting PRs work much better. Not only does such code often need more review, but you also typically have a smaller group of people who know the whole codebase, and are much better central repositories of knowledge. The review process is also a much more effective way of doing collaborative decision making in an open source project IMHO.

Post reply on HN