No code reviews by default • Hacker News Board

Re: No code reviews by default

#151

Post by baron816 » Wed, Jan 05, 2022, 12:03 AM UTC

rdw wrote: post #45

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 post sounds like wisdom, but after looking at it hard, it seems that the point is no more than "peer review processes are popular". Not to say that code review is bad in any way (I do it at my current role), but, this argument for them is not very good. One downside of code review is reduced velocity. There are teams out there who use code review so effectively that they never ship big fuck-ups to production. Th…

I'd argue that social proof often makes for a good argument. Perhaps not the best argument, but still a good one. "Best practices" still carry weight. Of course you can make an argument that the thing everyone does is wrong, and win. That's happened millions of times in this industry. But if the argument is nearly strictly between "this is what everyone does" vs "this is what I do", the former is going to win.

Re: No code reviews by default

#152

Post by Clubber » Wed, Jan 05, 2022, 12:07 AM UTC

seattle_spring wrote: post #133

Clubber wrote: post #62

Earlier quoted context omitted.

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.

You've had some bad experiences with QA people. The people I've worked with are methodical and work like the terminator to find bugs. One guy was so good he could even write queries against QA to show me where the data was inserting / updating improperly. A good QA person is worth their weight in gold.

In my experience, it's hard to have a dev as a good tester. I'm ok at it but not as good as a pro QA person. Good devs look for shortcuts and efficiencies, which is the opposite mentality for good testing.

Re: No code reviews by default

#153

Post by LenP » Wed, Jan 05, 2022, 12:07 AM UTC

Interesting read, and it may be working well for Raycast, but it didn’t resonate with me. The main drawback of code reviews, as they correctly note, is a lack of velocity due to engineers looking at code review as a “not my real work” thing. That’s a cultural issue in my opinion.

At my current startup, we are trying something different: the code reviewer fetches the feature branch and writes tests for the change as part of the review. As ridiculous as that sounds, it’s been working better than we’d imagined.

First, it’s really forced us to design modular and testable APIs. Separation of concerns at the file or library level gives a false sense of good, modular, reusable design. However, having an engineer write a test for your implementation (essentially a second client to consume your interface) really puts this to the test (!). The quality of feedback emerging from this process is often much richer than what a “skim the diff” code review offers.

Additionally, writing the test suite is an “unavoidable” task for engineers. This works two-fold: firstly, we don’t put it off as an “end of the day” item as with traditional code reviews on which you can spend as much or as little time as you have (it’s a ‘compressible’ task, unlike writing tests). Secondly, people send smaller, focused, easy-to-test PRs as a courtesy to the reviewer - and perhaps in part due to what we call MARBs... mutually-assured review-bombs :)

Re: No code reviews by default

#154

Post by baskethead » Wed, Jan 05, 2022, 12:08 AM UTC

tombert wrote: post #127

baskethead wrote: post #114

Earlier quoted context omitted.

I don't know what your point is. You would have gotten in trouble whether or not code reviews were present. If you're saying that code reviews should not be default, then the entire blame would rest on your shoulders anyway. 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?

> 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 because there's not enough back story. Generally "punishing" coders for bugs isn't good policy unless it's particularly egregious. If you were told that your code was sloppy, and if your performance review was affected by it, maybe it was sloppy code? Like I said, there's not enough back story.

If it was an innocuous bug or if it was a hard-to-determine bug, then I don't think you should have been written up, but again, there's not enough backstory to determine that.

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. It does sounds like they were ineffective code reviewers. If your code was sloppy, they should have picked it up, and if it was a hard-to-detect bug, neither you nor the code reviewers should be written up for it.

Re: No code reviews by default

#155

Post by dboreham » Wed, Jan 05, 2022, 12:14 AM UTC

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 :)

Re: No code reviews by default

#156

Post by aflag » Wed, Jan 05, 2022, 12:15 AM UTC

Abroszka wrote: post #37

yupper32 wrote: post #10

> 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.

I haven't seen any research that supports either. I wouldn't be surprised to learn that code review does nothing other than share knowledge, or to learn that it reduces bugs by 50%. I just honestly have no idea, I do code reviews, because we do code reviews. In fact I have seen alarmingly little research about code management. Code review, standup, agile, etc. does any of it do anything useful? I only came across ane…

I don't have a measure of how many bugs it prevents. But I've caught a few in code reviews and people caught a few of mine too. So, it does prevent some.

Re: No code reviews by default

#157

Post by jeffbee » Wed, Jan 05, 2022, 12:21 AM UTC

kubanczyk wrote: post #150

jeffbee wrote: post #122

Earlier quoted context omitted.

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…

Let's say we collaborate on the code below. I'll change line 1, you'll change line 5, we'll merge both changes and then we'll see how Perforce makes the code run successfully "without conflict or even awareness of other person". 1 x := 0 2 x++ 3 x++ 4 x++ 5 if x > 7 { 6 fail_horribly_at_runtime() 7 }

Explain how you think git is different in this regard.

Re: No code reviews by default

#158

Post by abeppu » Wed, Jan 05, 2022, 12:25 AM UTC

atomashpolskiy wrote: post #120

The problem is not with code reviews but with how adversarial they have become. Instead of making the best attempt to comprehend and embrace the author's style and intention and finding compelling arguments for the question "why this code is probably fine and should be merged as-is?" people nit on all kinds of stuff, mostly highly subjective. Along the way they don't actually catch that many problems or bugs but inst…

> Instead of making the best attempt to comprehend and embrace the author's style and intention and finding compelling arguments for the question "why this code is probably fine and should be merged as-is?" ...

I don't think we should take it as given that the goal should be to approve the code in the originally written form. In some sense, I think if you're in an environment where you're going to be doing PRs for every change anyway, if your code is consistently perfect when you cut the PR, then probably you've cut it too late?

What if instead we frame PRs as the basis for an artifact-based discussion about how to do something. The code needs to exist in a complete enough form to provide a concrete basis for discussion. But if you see your reviewer as a resource and collaborator, who might have ideas or insights that you didn't, then ask for those ideas or insights a bit earlier, before the code is polished.

The best PRs aren't the ones that catch a bug. The best PRs offer some criticism or question or suggestion that allows you to reframe an abstraction or refactor something to be simpler, more flexible, more testable, more readable, faster, whatever, and from which the original author learns something. Your code might not have had a bug before, but that doesn't mean it cannot and should not be improved.

Circling back to 'trust' though, this approach does require a degree of trust. The reviewer can't be focused on nits and gotchas; more minor things are inevitable if our colleagues pull us in sooner, but we have to trust that they'll get sorted out.

Re: No code reviews by default

#159

Post by efxhoy » Wed, Jan 05, 2022, 12:26 AM UTC

There is obviously a tradeoff here with the optimal amount of code review that varies with impact and difficulty. Dogmatically doing and waiting for an in depth review on a tiny and likely insignificant change is silly. Blindly pushing big db migrations is also silly.

Re: No code reviews by default

#160

Post by lmm » Wed, Jan 05, 2022, 12:28 AM UTC

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?

For the same reason that people write code they'll be responsible for: it's their job. Code review works when it's seen as equal priority and equal importance to writing code.