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

81–90 of 315 posts

Re: No code reviews by default

#81

Post by canyonero »

I could see this working well for teams that are engineering led and where customers don't expect a ton from the software. If you're trying to ship fast and get to market quickly, then fine, skip the tests, skip the code review.

Once you have lots of engineers shipping many different apps and need to work within and across teams. This system is not going to be fun. When a nasty bug ships and you're responsible, you'll wonder "should I have requested review for that one?" When one of your colleagues ships a few bugs that force you to paged during your on-call. This system is likely to erode trust on both ends. IMO, a no code-reviews model is going to stunt junior engineers career growth as they will not be performing what is common practice on software engineering teams. It's also going to keep out many other stakeholders who may wanna weigh in on the software you're delivering, be it UX, Accessibility, Security, Documentation, Product experts. Pull requests keep others on your team in the loop and educated.

I agree that this model gives you the advantage of speed, but I don't think it builds trust. I trust my colleagues and we do pull requests. I don't feel like I'm missing trust because I can't push to `main`. Code reviews have very little to do with trust. They serve as a communication tool and serve a tool to give the best possible experience for customers and provide an opportunity for alignment with our colleagues.

Re: No code reviews by default

#82

Post by dromtrund »

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…

> drive-by take-down by someone with none of the context.

If this is an issue in your workday, you should bring it up with management or the individual rejecter. If someone has the authority to kill PRs, they should also be required to put in the effort to understand it and/or be available for discussion at an earlier stage of the development process. PRs shouldn't ever be rejected without mutual agreement - neither internally in an organization, or in an open source project.

Re: No code reviews by default

#84

Post by ipaddr »

kps wrote: post #70
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. 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?

Re: No code reviews by default

#85

Post by PeterWhittaker »

We're very small (3 full time devs, a mostly full time tester, a part time student, and another student temporarily full time), and the three full timers are pretty seasoned, so this may not apply universally, but....

We don't review code as a matter of course. But if anyone of us writes code that doesn't pass our personal smell test, or that we think it won't pass someone else's smell test, or , screens are being shared and ideas tossed about.

We almost always , in pretty short order.

So it's sort of JIT/OnDemand code reviews. Kinda.

Re: No code reviews by default

#86

Post by that_guy_iain »

> Pull requests don't prevent bugs.

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. Also, if you do see a comment about bugs they'll often be disregarded. This drives me up the wall when I've previously pointed out bugs and they've been found to be actual bugs and had to be fixed.

I think the biggest problem with code reviews is it often becomes adversarial and people want to come out on top. I've had it more than once that someone has suggested another way of doing something, when I asked what the benefit of that way was or what was the downside of the way I was doing it no answer was forth coming however I was expected to implement their way. This becomes enraging when you point out flaws in their way but they can't find any benefit of their way.

As well as, often people just don't want to do the extra work. You point about a bunch of small improvements. It's a pain.

You end up with people asking for code style changes even tho there is a code style and the changes they're asking for aren't in those guidelines-

In my opinion, code review is a massive waste of time that is often done purely as cargo cult. People know it's a good thing to do but people have no idea how to code review and what is and what is not benefical during code review.

Re: No code reviews by default

#87

Post by marcosdumay »

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…

A formal review step where you accept or reject the code, yes, is always there because of lack of trust.

Informal reviews can have a lot of different shapes, and provide a lot of different benefits. But anything that is a stop-the-process activity is there because you expect quality problems from the earlier steps.

(That said, no good developer trusts oneself. So if you don't have anything on your process that makes you trustworthy, lack of trust is the correct approach.)

Re: No code reviews by default

#89

Post by xmlninja »

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 trust you with my life but do I also trust you with my money and my wife?

Re: No code reviews by default

#90

Post by kcb »

watwut wrote: post #41
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.

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

It's not as ridiculous as it sounds as long as the testing and verification happens as part of the PR.
Post reply on HN