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

201–210 of 315 posts

Re: No code reviews by default

#201

Post by plorkyeran »

sanderjd wrote: post #98
erpellan wrote: post #38

Earlier quoted context omitted.

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…

Is this a common practice? I've never worked under a code review process where the accept / reject decision was being made by "drive-by" reviews from "someone with none of the context". It has always been team members with a lot of context reviewing each others' code.

I've never even worked with a code review process where outright rejecting PRs is a normal part of the process for anything other than external contributions. The normal process is more of that if you think a PR is a bad idea it's up to you to persuade the author of that. You'll certainly sometimes have the author withdraw the PR without being truly convinced, but something like "I don't like this. Rejected." would just be disregarded if you can get other people to approve it.

Re: No code reviews by default

#203

Post by jayd16 »

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 it wasn't about trust you could just read the commit history. Its certainly about trust.

Re: No code reviews by default

#204

Post by jayd16 »

joombaga wrote: post #145
snarf21 wrote: post #43

Earlier quoted context omitted.

I think it is trust in the trust and verify sense. Meaning, we trust each other to do our best but let us verify that we are all on the same page and aren't breaking things or doing something harmful.

What is the "trust and verify sense"? I've been told this by managers before but I've honestly never understood it. "Trust" to me means "be willing to take action without further examination of the facts at hand". If I trust your code I wouldn't review it. But I don't trust your code, and I don't trust you to have taken all of the appropriate considerations when you were writing it. What's the difference between "tru…

Its a Reagan quote but in this context it means that you trust their intentions, effort etc, but its still worth checking.

Re: No code reviews by default

#205

Post by justrudd »

ec109685 wrote: post #200

I wonder how much refactoring is _not_ done because of the need for a code review. In the era before code reviews, I used to go to town on my codebase when I saw something I wanted to fix. Are folks more hesitant to make such changes now because they are afraid of the burden on the code reviewer?

Not really. But if I start doing a major refactoring while working on a new feature, I’ll generally cut a new branch, move the refactoring over to it, and do a PR of just the refactoring. That makes it easier for the reviewer to only have to look for degradation of current functionality. While the refactoring is being reviewed, I’ll then usually start working on the new functionality on top of that branch.

It can get a little unwieldy if I’ve got a lot of refactoring going on which I currently do. I inherited a codebase that never said no to any “hey. That’s a neat blog post. Let’s try that way for this feature”.

Re: No code reviews by default

#206

Post by touchpadder »

This may work in a small colocated team where everyone is experienced/smart cofounder and knows the codebase by heart but in a larger team where new devs keep joining it would be a total disaster. Undoing errors that were pushed to master is a bigger waste of time than doing reviews. If all developers are good the reviews usually take a minute and only a quick scan for anything alarming.

In my private projects when going again over my code in i.e. Source Tree I'm tempted to stage stuff that is messy with many comments and should be rewritten hoping I'll come back to this another day. The temptation is too big when there's no one looking. The fact someone will be reading your code encourages a proper cleanup before creating a PR.

Re: No code reviews by default

#207

Post by jayd16 »

I don't think every single commit needs to be reviewed, especially early on in a dev cycle especially if there is no production environment to even screw up yet. In that sense, I think they should be optional.

Code moving to prod gets reviewed though.

Re: No code reviews by default

#208

Post by mslate »

azov wrote: post #129

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.

Early Facebook is one notable example.

Re: No code reviews by default

#209

Post by munchbunny »

lewisl9029 wrote: post #144
jolux wrote: post #42

Earlier quoted context omitted.

Sure, I mostly meant automated processes. They won't always save you from yourself but doing blue-green, having an extensive test suite, etc, are all things that will help reduce the risk of a bad deployment.

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.

Re: No code reviews by default

#210

Post by tshaddox »

detaro wrote: post #121
erpellan wrote: post #38

Earlier quoted context omitted.

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

> If that's a regular issue that's a culture problem.

Couldn’t one also argue that if code reviews are routinely catching problems mentioned earlier in the thread (misinterpreted requirements, conflicts with other WIP, etc.) then that’s a cultural problem? It just seems odd to assume that the person assigned the task couldn’t possibly be expected to routinely avoid those problems, but that adding rigorous code review could routinely avoid them.

Post reply on HN