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

141–150 of 315 posts

Re: No code reviews by default

#141

Post by RegW »

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 ... they have become.

Perhaps "banal" is the word you are meant to use.

There's a saying about that a 10 line PR will get 10 comments, but a 1000 line PR will get a "looks good!".

Of course this is not really what is happening. A 10 line PR can be completely unintelligible and a 1000 line PR might be a joy to read.

Once the code base has become a ball of mud, no one really bothers to review anything properly - it's going to take as long as writing it yourself. So perhaps, we get picky about some worn out code style point (incorrect indent etc) just to show we are alive.

Re: No code reviews by default

#142

Post by kubanczyk »

commandlinefan wrote: post #20
mixedCase wrote: post #3

I'm glad this works better for them, or at least they feel that it does. I'd also bet that they're either a small minority of teams that jell so well that they manage to scale a codebase without it turning into a disgustingly inconsistent mess; or that indeed their codebase qualifies as such or is still too small for the effects to be very visible. Code reviews are not a tool intended mainly to catch bugs. Types, aut…

> Code reviews are not a tool intended mainly to catch bugs Well, not to detract from your point (I agree), but what little research has been done in this area suggests that code reviews are actually one of the best ways to catch bugs: https://kevin.burke.dev/kevin/the-best-ways-to-find-bugs-in-...

The source actually says that informal code reviews are very poor way to catch bugs, while formal code reviews are one of the best.

I don't have "Code Complete" at hand to follow up on that, but I strongly suspect that the formal code review was defined in this research as reviewing the entire codebase line-by-line on meetings. As opposed to reviewing just a single change to the system.

Can you/anyone shed some light on this?

Re: No code reviews by default

#143

Post by coffeefirst »

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…

Fair, and I've seen this, but usually it's because the reviewers are treating "I would personally prefer to do this another way" as blocking feedback, when it isn't.

Good reviews can take a light touch, in part by differentiating non-blocking ideas and suggestions from "hey I think this is a bug."

Some of my favorite reviews of all time actually don't change a single line of code, but the questions reviews ask zero in on what's going to confuse the next developer so it can be documented appropriately.

Re: No code reviews by default

#144

Post by lewisl9029 »

jolux wrote: post #42
jaredsohn wrote: post #35

Earlier quoted context omitted.

For many developers, people merge their branches directly to production so the pull request review is where that checking happens.

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 different human can understand and maintain the code.

In fact, if we manage to catch a bug in a code review that didn't also trigger our automated systems to alert us, that should probably prompt us to look into how we can expand the coverage of that automated system, similar to what we'd do in a postmortem if the bug wasn't caught in code review and reached production, because we can't rely on that happening the next time.

Re: No code reviews by default

#145

Post by joombaga »

snarf21 wrote: post #43
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…

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 "trust but verify" and "don't trust, verify"?

Is the phrase "trust but verify" an attempt to avoid damaging self-esteem? There seems to be some emotional/ego-based attachment to the concept of trust.

Re: No code reviews by default

#147

Post by zzzeek »

that_guy_iain wrote: post #112
zzzeek wrote: post #102

Earlier quoted context omitted.

> I remember once I was giving a talk on code review and I said that code review doesn't prevent bugs and people were shaking their heads. Everyone hangs on to this one thing, but if you look at the majority of pull requests that have been approved you'll see a lack of comments about pontential bugs. that's because the UX for github PRs sucks. Use Gerrit instead. It's not as pretty but it's super productive. > I thin…

> that's because the UX for github PRs sucks. Use Gerrit instead. It's not as pretty but it's super productive. That won't affect the quality of what people find and comment about. > that's not a problem caused by code review, it's caused by people who don't know how to work with others (or otherwise simply refuse to). to the degree that the code review process shines a light on this, that's good news, because that's…

sounds like you just work at awful jobs. i wouldn't stay at companies with the problems you describe above. wishing you good luck.

Re: No code reviews by default

#148

Post by mulmen »

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…

I tend to scan most reviews for the team even if the code review itself is not directed to me (we use a round-robin/domain expert routing method). It helps me understand what my teammates are working on and give me a better understanding of our system. I can't imagine anyone interpreting that as a lack of trust. Is looking at our source code inherently distrustful?

Re: No code reviews by default

#149

Post by jiggawatts »

alecbz wrote: post #27
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…

Yeah, IMO this is like a publisher saying "we don't have editors: we trust our authors". The intended ethos of editors and code review is that mistakes and imperfections are the norm, and you need a second pair of eyes to iron them out. That said, I do sometimes encounter individuals or cultures that seem to view code review more as a mechanism for catching abnormal/unexpected mistakes than as a normal part of the pr…

Something that I've discovered is that as the volume of text that I write goes up, the errors increase. More experience hasn't resulted in fewer typos. On the contrary, I make far more small mistakes now than I used to.

It's precisely because I've learned to read and write so fast that I "skip over" the simple parts of sentences, making me oddly blind to certain categories of spelling errors!

Especially when I've gone over the same text over and over, I'll have memorised it to the point that I can skip entire sentences.

This is why you need someone else to step in and read the text for the "first time" and not skip anything.

The same applies to programmers and code review.

Re: No code reviews by default

#150

Post by kubanczyk »

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…

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  }
Post reply on HN