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

121–130 of 315 posts

Re: No code reviews by default

#121

Post by detaro »

erpellan wrote: post #38
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…

That final, 1 word sentence is one of the major problems of PRs. An engineer might spend several hours really thinking through a problem, talking with colleagues, whiteboarding options and coming up with a workable solution that addresses all the obvious issues and a bunch of non-obvious ones. Only for a drive-by take-down by someone with none of the context. It's a totally asymmetric investment of time. Even helpful…

> Only for a drive-by take-down by someone with none of the context

If that's a regular issue that's a culture problem. Starting with "if you've been talking with colleagues about your problem, why is someone with no context reviewing the result?", people not investing time in reviews, people doing "take-downs" instead of asking questions if they don't understand things, hold up merge for non-urgent concerns ...

> Even helpful review comments might only take a minute to write but a day to incorporate.

Either the feedback is worth the investment of the day of time, then no problem, otherwise it's not that important and doesn't need to be done (or depending on what it is can be done later or ...)

Re: No code reviews by default

#122

Post by jeffbee »

nix0n wrote: post #101
jeffbee wrote: post #32

Earlier quoted context omitted.

It's a great example of how Linus has a massive blind spot for the failings of the git model. He's "sorry" they use Perforce. Git is "better than everything else out there" but he doesn't mention Perforce. And yet, the Perforce model is the one that got Google to be one of the fastest-moving organizations on the planet, with the biggest code base.

> 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 other people, politics, special write access groups, are all non-issues that do not sound familiar to Perforce users. If two people are using Perforce they can both work on changes to the same files taken from the main branch (trunk) and submit them separately without conflict or even awareness of the other person. As a bonus this is all dramatically faster in Perforce than in git for large repos.

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.

Re: No code reviews by default

#123

Post by windows2020 »

I've seen a number of flavors of code reviews in practice:

  * Superficial - Omission of optional lagging comma
  * Valid in context - Quadratic logic on small finite set
  * Ensure intention - Task not completed/completed with adverse effects
Each takes progressively more effort, skill and context knowledge. There's a constant battle between resiliency and efficiency. There also tends to be a gradient of skill involved. Context is king when it comes to an opinion on this.

Re: No code reviews by default

#124

Post by 0xbadcafebee »

> we trigger an internal release every night [...] This allows us to test new changes within 24 hours [...] This workflow helps us to consistently ship updates every other week.

If instead of testing 1x a day, you tested 3x a day, you'd debug 3x faster, meaning you could ship 3x faster. Now imagine testing 100x a day. Every merge to main should test immediately and ship immediately. That's how you build trust, speed, and quality.

Your developers should be going, "fuck. I'm not merging this piece of shit yet and breaking production. I need more tests." Some people will say that waiting until you have enough tests is crazy because it's not fast enough. But the only way to get fast is to have trust, and the only way to have trust is more quality. More quality means less bugs, which means less cost and delays, which means shipping faster, cheaper. Go slow to go fast, shift left.

Tests are better than code reviews. With a code review, you're only reviewing one change at one point in time. 300 commits later, you're not reviewing how the 1st commit is affected by the 301st commit. But a test around the 1st commit is verifying against the 301st commit. Tests are the eyeballs that never stop looking.

Re: No code reviews by default

#125

Post by fragmede »

ipaddr wrote: post #84
kps wrote: post #70

Earlier quoted context omitted.

> Only for a drive-by take-down by someone with none of the context. Then write it down. If the code reviewer can't follow what's going on, what hope is there for the new hire looking at it six months from now?

Because looking through 6 month old prs are what new hires are doing to learn the codebase?

Running through `git blame` and looking at the commit message, the PR, and the PR comments is a very practical way to learn about a codebase! Beyond some high level code organization stuff that exists in a readme, learning a codebase is really about getting a history lesson of how the product evolved, the company pivoted, and the team re-org'd.

It'll be 6-months if you're lucky. Try figuring out why there's a particular "if" clause, one or two or four years later. Software maintenance, especially when the original author has moved onto another team/organization/company/career, is a frustrating art of almost remembered stories and hoping that you can figure out Chesterton's fence, lest it become Chesterton's barbed wire. The worst is when you fix a small bug with code and introduce an even bigger big in the process.

Re: No code reviews by default

#126

Post by deckard1 »

Abroszka wrote: post #74
yupper32 wrote: post #65

Earlier quoted context omitted.

Sorry, are you saying that it's possible that code reviews cause more bugs than they solve? That's absurd...

Why would it be absurd? If there is a process to reduce risks, then people take more risks. I have been guilty of submitting code review when I'm not 100% sure it's perfect, just because I know that there is a code review process. So this definitely exists, not sure how common it is though.

Also, the trick to code reviews is to leave in a few low-hanging obvious bike sheds. The reviewers will tell you what color to paint them. Done.

I'm being slightly sarcastic. But not sarcastic to the point that I haven't done just that. Just as the nail that sticks out gets hammered, the code that is too perfect gets increased scrutiny.

Re: No code reviews by default

#127

Post by tombert »

baskethead wrote: post #114
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…

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 approving the PR should share responsibility for it.

Re: No code reviews by default

#128

Post by brokencode »

ReleaseCandidat wrote: post #67
brokencode wrote: post #49

Earlier quoted context omitted.

I don’t think I’ve ever reviewed code from a new hire that didn’t have multiple problems or style issues that needed to be addressed. That’s also actually true for code from most experienced devs too. And it’s rare for my code to get through code review with no issues found. So bottom line, I just don’t think this would work for most teams and developers.

> I just don’t think this would work for most teams and developers. Most teams and developers don't do code reviews because the only person that could do code reviews would be the developer himself.

Do you mean to say that most developers work alone, so don’t have anybody to review their code? That’s probably true for hobby projects and the like, but companies rarely have teams of one working on anything.

Re: No code reviews by default

#129

Post by azov »

Coming up next: “We don’t use source control. Git just slows us down. Our engineers don't want to spend time writing commit messages when they can be writing features instead. We keep all our code in a shared folder and we trust our team members to not mess it up!”

Well, this can be done. Lots of software was written before code reviews or source control existed. But I’m not longing for that time.

Re: No code reviews by default

#130

Post by codeptualize »

Good CR's are great, bad CR's are an incredible waste of time and you might as well not do them at all.

We had a similar system where you trust people to know when you can skip the CR, worked great. We also ranked CR comments on importance, focussed on preventing issues, and avoid subjective discussions as much as possible. It meant CR's were mostly about bug catching, sometimes constructive conversations weighing pro's and cons, and other times making suggestions that may or may not be applied.

It's so much nicer than a dogmatic nitpicking CR culture that completely forgets about what's valuable and not. Maybe it's my relative small sample size but I feel they always go together.

Post reply on HN