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

161–170 of 315 posts

Re: No code reviews by default

#161

Post by etchalon »

The only time I make anyone on my team submit a code review is if they're a junior developer, and we're actively trying to make them a better developer.

Otherwise, my experience with code reviews has been that it's one developer telling another developer how they'd approach the problem, or their preferred style, or some such nonsense, with the originally submitted code good enough to get the job done.

Everything else this author says rings true to me when code reviews are a universal practice.

They slow everything down, and only occasionally is that slow down worth it.

Re: No code reviews by default

#162

Post by SkyPuncher »

I feel like this article is written with a bunch of false dichotomies. You can still have code reviews and still have trust.

We're required to do code reviews for SOC II compliance. Even if we werent, I'd likely still encourage my team to do them.

I trust everyone on our team. They're all extremely talented and competent. They could all ship things to prod everyday without a peer code review and I doubt we'd notice any difference in quality.

The reality is code reviews serve way more than making big brother happy. They help to avoid silos. They help to gain perspective. They help people mentor and grow. They help the team build a collective understanding of how to work effectively.

Re: No code reviews by default

#163

Post by etchalon »

marcinzm wrote: post #58
klabb3 wrote: post #25

Earlier quoted context omitted.

Meh. If 90% of the time it's a mindless ritual there is probably a better way to achieve the same goal. Code reviews are a huge time sink, especially for unimportant style- and naming nits. Code review as mentoring can be great, if it's a directed 1:1 effort. Usually, it is not.

>Code reviews are a huge time sink, especially for unimportant style- and naming nits. This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on. This…

There's a large number of things in any decently complicated system that a "style guide" can't cover and that there are dozens of potential ways to solve.

Re: No code reviews by default

#164

Post by mike_d »

mumblemumble wrote: post #24
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…

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…

> do code reviews in order to make sure everyone understands the code.

I like a hybrid "the person who wrote it can't deploy it" model. You get an informal code review and two sets of eyes/brains to the extent that the deploying engineer feels like they understand it enough to shepherd it into prod weighted against how important the component and the scope of the change is.

It self scales from "copy change on the website yolo" to "this involves two new racks of servers and a database migration, I'm going to need to spend a week understanding it to the point I can make a plan."

Re: No code reviews by default

#165

Post by passive »

I think a key thing here is that they are only doing releases every other week.

So when changes skip a code review, they are still subjected to a good amount of peer testing before they ever reach a customer.

I think that's a reasonable approach in the situation, and I can see why they have gotten good results from it.

Re: No code reviews by default

#166

Post by tombert »

baskethead wrote: post #154
tombert wrote: post #127

Earlier quoted context omitted.

> Are you saying you wish you got in less trouble at Apple and the code reviewers should have gotten in more trouble for missing YOUR bug? If “my” bug was a result of sloppy code, my PR should have been rejected. I don’t really think anyone should get in trouble for a code bug (at least in a majority of cases), but yes, if this bug was obvious enough for me to be in trouble for not catching it, then the people approv…

If code reviewers share responsibility for the code they review, why on earth would anyone review code? Everyone would stop reviewing other people's code. Then you're in the same situation where no one reviews code, and then you yourself would be responsible for your own code by yourself. It sounds like you think you shouldn't have gotten in trouble for having bugs in your code. I have no comment on that situation be…

> If code reviewers share responsibility for the code they review, why on earth would anyone review code?

Ok, and if there are no shared responsibility, then what’s to stop the reviewers from just mindlessly clicking “approve” to shut the person up? I know for a fact that that happens all the time.

> If you were told that your code was sloppy, and if your performance review was affected by it, maybe it was sloppy code?

Sure, but again, isn’t “sloppy code” the lowest hanging fruit for a code reviewer? If the PR process doesn’t spot that, what exactly does it offer?

> But to say that your responsibility is lessened because it was blessed by code reviewers and they should share in your punishment, is, frankly, immature

I think we are just going to have to agree to disagree on this. If you are signing off on something, you are attaching your name and credibility towards it. I didn’t really want any of the reviewers to get into “trouble”, but i do think they should share a percentage of the responsibility on breakage if I am going to get in trouble if they signed off on something that was “sloppy”.

Re: No code reviews by default

#168

Post by klabb3 »

marcinzm wrote: post #58
klabb3 wrote: post #25

Earlier quoted context omitted.

Meh. If 90% of the time it's a mindless ritual there is probably a better way to achieve the same goal. Code reviews are a huge time sink, especially for unimportant style- and naming nits. Code review as mentoring can be great, if it's a directed 1:1 effort. Usually, it is not.

>Code reviews are a huge time sink, especially for unimportant style- and naming nits. This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on. This…

> since clearly there's strong disagreement on coding styles and conventions

The same people who are perfectly happy to do a back and forth, adding 3 days of code review time, would never go in and refractor existing code to "fix" stylistic issues. It's a cognitive bias that we think everything is up for philosophical debate in reviews.

Btw, I'm one of those people who drove stylistic debates in an organized fashion. I learnt that you simply can't police everything, even if it we pretend that it's a good idea to do so. It's much better to share ideas and have them spread through voluntary mechanisms, such as dedicated tech talks, mentoring, postmortems and other deliberate knowledge sharing.

That said, automated enforcement of pure formatting can work well if there's standard tooling, e.g. gofmt and rustfmt. But contrary to reviews, that's a way to suppress debate during day to day business.

Btw, my experiences are from a mega corp, not claiming it's true for every org.

Re: No code reviews by default

#169

Post by tombert »

dboreham wrote: post #155
tombert wrote: post #104

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…

> worked for Apple Well at least it had one positive effect :)

I guess from a resume-fuel perspective. I don’t think I was a great fit for the company.

Re: No code reviews by default

#170

Post by tick_tock_tick »

jeffbee wrote: post #122
nix0n wrote: post #101

Earlier quoted context omitted.

> the Perforce model I've used both Git and SVN enough to understand how a VCS might change how an organization creates code, but I've never used Perforce. What is "the Perforce model"?

In Perforce the concepts are different. You have a client, with a view that includes a subset of the repo, and you have changelists which incorporates your related edits. When done with a changelist, you submit it to trunk, usually, because while you can branch a Perforce repo, you mostly don't need to. Basically all the stuff in the first half of the talk where he whines about stepping on toes, conflicting with othe…

> In this talk Linus demonstrates that he believes git is a state-of-the-art SCM system, without being familiar with the actual state of the art.

I don't think that's very fair Perforce is mostly just a shitty subset of git's features that some people pretend is sufficient. You can use git almost the same as Perforce and have almost as bad of an experience as using Perforce directly.

Post reply on HN