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

171–180 of 315 posts

Re: No code reviews by default

#171

Post by sharken »

ljm wrote: post #14
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 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…

It's the same in the financial sector, you can't just push code without a review.

However, the review process is far from perfect and can create a false sense of security.

To review a piece of code, that code should ideally be small in scope. For larger pieces it's rather common that the reviewer don't have enough time to do a good job.

If there were better incentives for doing a review, then it would greatly improve the situation.

But i haven't seen that yet.

Re: No code reviews by default

#172

Post by detaro »

jeffbee wrote: post #157
kubanczyk wrote: post #150

Earlier quoted context omitted.

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.

You're the one that claimed Perforce was better than git and could do this "without conflict or even awareness of the other person", so why don't you explain how git is worse? (I've never used Perforce and am genuinely curious what clever kinds of things it can do around merges, but more detail than "it's state-of-the-art and git is not" would be useful)

Re: No code reviews by default

#173

Post by klabb3 »

brokencode wrote: post #49
klabb3 wrote: post #19

Earlier quoted context omitted.

The idea is not that everyone has to be omnipotent, it's that you trust people enough to send for review and ask for help when appropriate , instead of always. I.e. when you're new to the code base or language.

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.

New hires definitely fall in the category of where code reviews (and many other forms of knowledge sharing) make sense. That's not what this is about, it's about that they become mostly ritualistic when mandatory, the same way that standups and many meetings are often useless, especially when mandated for no clear purpose.

Re: No code reviews by default

#174

Post by bhawks »

Two things I love about code reviews is the whole team has a general awareness of the changes happening in the code base. The other thing is how as the team grows/changes cultural norms ranging from testing, style, interface design and so on gets established, normalized and strengthened.

Having worked in places where there were no code reviews the software development culture was all over the place. I wouldn't go back.

Re: No code reviews by default

#175

Post by adam_arthur »

There are other ways to solve the problems the author calls out.

Reviews being slow can be largely solved with review SLAs/automation to ping reviewers. Code review themselves can be quite fast if you know the code author well. Some people you can trust to make high quality changes and the review covers more high level/directional questions. Others are more junior/haven't built the trust yet and require more scrutiny.

But having done a large number of reviews, it really doesn't take long to do even an in-depth review, if you're already familiar with the area of the code in question. The goal of a review is not to test the code for the code author, or to identify bugs for them... which is where things could really slow down. I used to do 10 or so reviews a day and it would take an hour or so to get through them, while still writing a lot of commentary on them, where applicable... not a rubber stamp.

I know the author mentions not wanting there to be a lack of trust, but its pretty evident they'll realize the need to build trust first as they scale.

There is also an aspect of the code author writing code to a higher standard when they know it will have to go through another set of eyes before being merged. The best people can set this standard for themselves regardless, but it's unlikely that you could run an org at scale on this premise without a drop in code quality.

It is an interesting idea to try to run a company only hiring people that have the skills/attitude to be implicitly trusted. I think this approach is very feasible at a small scale if you can maintain very high hiring standards and are able to nab mostly 10x style engineers. These people will make mistakes, but will recognize them and self correct. Then you could just have people contributing under a guiding set of principles/standards, and "retroactive" review to ensure standards being followed.

But almost certainly impossible to hire at this quality/bar at scale.

Having said all that, I did work in a startup where in the early days certain people had write access and pushed directly. It mostly worked well at that small scale, but lots of technical debt developed in certain areas of the product that took years of effort to reverse/fix.

Re: No code reviews by default

#176

Post by jeffbee »

detaro wrote: post #172
jeffbee wrote: post #157

Earlier quoted context omitted.

Explain how you think git is different in this regard.

You're the one that claimed Perforce was better than git and could do this "without conflict or even awareness of the other person", so why don't you explain how git is worse? (I've never used Perforce and am genuinely curious what clever kinds of things it can do around merges, but more detail than "it's state-of-the-art and git is not" would be useful)

No, it is Linus who makes a bunch of claims about how Perforce is worse, in the video.

If two people edit the same line of code based on the same original version, that's a conflict and there's nothing any SCM can do about it. Human must merge. But there's no reason that several developers can't work on different functions or other disjoint areas of the same file, unlike what Linus is claiming in the video.

The video is essentially a long straw-man argument where Linus takes shitty part of CVS and SVN, which were terrible, and applies them to Perforce, which it's clear he has never used.

Re: No code reviews by default

#177

Post by detaro »

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…

> The problem is not with code reviews but with how adversarial they have become.

I've never really seen that. What do you think is the reason for people thinking that's the thing to do?

Re: No code reviews by default

#178

Post by keithnz »

I feel their "responsibility" and "trust" argument is maybe a little oversold here, but their final paragraph is right on the money.

I would argue a better title is "No rules/process by default". Which doesn't mean you go wild, having a strategy to develop software is very important, but strategies need to be customized. It means you apply just enough and constantly adapt so you have just enough of whatever you need to make sure development works smoothly. People will say PRs improve communication, can help prevent problems, help understand what changes are going into the code base etc, but all of those things can be done in many different ways, and depending on the group of people, there can be more effective ways of achieving that. So adopt what you need ( which may be PRs, or some variation ). But whatever you do, don't go overboard with justification.

The dangers with overselling and defending your justification for doing or not doing something can be a problem. It also can mean you don't adapt quickly to change. By overly emphasizing your approach being about "responsibility" and "trust" you kind of make out that PRs are about a lack of trust and responsibility, which is incorrect and may prevent you from adapting because you have misattributed a quality to a practice/process. This often happens when people go from A (problematic) -> B ( much better). They attribute too much weight to what makes B better than A when they may need to change those things to go to C (even better). In my 40 odd years of observing software development, this is really common, especially for many devs with around 5-10 years experience (including myself). You find much better ways of doing things, and you start becoming advocates for those things. In reality, you always need to be adapting and questioning and always deconstructing and reconstructing your strategies about how to do software.

Re: No code reviews by default

#180

Post by joshuamorton »

dtech wrote: post #60
watwut wrote: post #41

Earlier quoted context omitted.

Ok, but many companies don't use such ridiculous process.

iirc Google does it, it's not ridiculous at all. You need a lot of automated tests and canary deployment to pull it off though.

Not really. Googler merges to HEAD, but systems aren't running directly from head.
Post reply on HN