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

301–310 of 315 posts

Re: No code reviews by default

#301

Post by ahtihn »

gunapologist99 wrote: post #298
ahtihn wrote: post #256

Earlier quoted context omitted.

If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.

This sounds similar to the peer review process in publishing. Typically, getting other people to actually agree to voluntarily review your work is indeed a significant hurdle if the requested paper/book/chapter/white paper/code review is lengthy . There is an easy way to resolve this: Request peer review for SMALLER chunks of code. Git makes this exceptionally easy -- it's certainly far easier than reviewing a large…

The problem is that small changes are barely worth reviewing. Also a series of small changes can obfuscate the larger context of all those changes.

Each small change on its own might make sense and be ok to a reviewer, but if you step back and look at the series of changes together there might be some issues that could have been noticed.

Re: No code reviews by default

#303

Post by Fishkins »

kubanczyk wrote: post #142
commandlinefan wrote: post #20

Earlier quoted context omitted.

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

My reading is that a formal code review is still focused on a single change, but with a more explicit/required process for review. This sentence in the article backs that up:

> Before any code at Google gets checked in, one owner of the code base must review and approve the change (formal code review and design inspection)

That's clearly referring to a specific change, not a review of the whole codebase.

I used to work for a large company. They have homegrown review software with extensive checklists of criteria the code must meet before it can pass review. The software won't let the code progress unless two reviewers go through and explicitly check off every box to certify it meets their standards. I'd call that formal code review.

I work on a smaller team now. We do consistently perform code reviews, but there's nothing actually preventing someone from merging without a review. We also have a list of things to look for when performing a code review, but nothing forces reviewers to look at the document regularly. That process is closer to an informal code review.

I still find bugs regularly when reviewing code. That's both true of other people's PRs as well as my own. I imagine there's a lot more variability with an informal process than a formal one, but IMO informal review is still useful. Code review of course also provides benefits other than bug detection.

I always review my own PR before passing it to someone else. Another commenter recommended that practice, and I heartily endorse it.

Re: No code reviews by default

#304

Post by healsjnr1 »

ahtihn wrote: post #256
mpweiher wrote: post #244

Earlier quoted context omitted.

> I want my code peer reviewed TFA: > Engineers [..] request reviews when they think it's necessary. Problemo solved.

If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.

> If code reviews aren't an expected part of the development process and there's pressure on delivering, it won't happen.

This hits at the heart of why I strongly agree with this post and think PRs and code reviews are ultimately destructive.

Reviews are going to get dropped _regardless_ of whether they are in the process or not. Pressure makes code reviews and quality worse, not better.

More than that, the need to "appear" to review is going to slow things down and can cause an exponential back up. I saw this happen about a mouth ago: 3 day release cycle got bogged down and then took _3 weeks_ to get a single release out.

Process goes out the window when pressure hits. You are then operating in foreign environment at the most critical point and your safety net of useless.

Your safety net when things need to go fast should be highly automated fast build and deploy, coupled with great test coverage, from unit to end to end.

The only exception might be if you work in highly regulated field where you legal required to have a very low Mean Time To Failure. In that case things move slow unless you are very well resources.

Otherwise (ie the majority of products) focus that effort on having a very low Mean Time to Resolve. If you absolutely need some form of quality assurance, Pair. Do continuous PRs and knowledge sharing rather than out of context after the fact reviews.

Re: No code reviews by default

#306

Post by cfer43432t »

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…

> otherwise it's not that important and doesn't need to be done

Except that this decision is often not up to the code author, because the tooling rejects for example committing changes until all discussions are resolved (see Gitlab for example) and some @holes make their quest for any reason not to resolve the discussions that they started until the code author writes the exact code with the exact words in the exact indentation with the exact architecture, etc that those @holes like, which makes the whole code review like a torture and the REAL productivity killer. Not to mention that this does not provide any improvement to the code base either in most cases.

Re: No code reviews by default

#307

Post by cfer43432t »

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 think the issue is not that people make drive-by issues, the problem is that people argue about things endlessly which does not bring any value neither to the customers nor the company, that is only for the vanity of the reviewer, and many reviewer get offended when someone does not take blindly their "wisdom" and for retaliation they block code commits endlessly which is enforced by automated tools like Gitlab.

Re: No code reviews by default

#308

Post by cfer43432t »

Nursie wrote: post #194
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 Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code. > Even helpful review comments might only take a minute to write but a day to incorporate. Then like all pieces of work, a decision must be taken whether that day is worth it, no?

That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.

Re: No code reviews by default

#309

Post by cfer43432t »

The biggest mis-conception about code reviews that it is for catching mistakes and improve quality. Well, you know what improves quality? QUALITY ASSURANCE. If you never heard about it, THAT is the issue, and your group/section/company should really educate itself if this is a new concept.

Also code review as a communication tool? Maybe yes, but again there are better tools for the job.

Re: No code reviews by default

#310

Post by Nursie »

cfer43432t wrote: post #308
Nursie wrote: post #194

Earlier quoted context omitted.

> Only for a drive-by take-down Code reviews aren't a "take-down", they're a process of helping each other produce better solutions and better code. > Even helpful review comments might only take a minute to write but a day to incorporate. Then like all pieces of work, a decision must be taken whether that day is worth it, no?

That's what YOU think. But in practice they produce sub-par solutions and worse quality in my experience. Code reviews are basically a virtual piss-contest where people argue endlessly, everyone trying to show-off their intellectual superiority. And reviewers (not code authors) often can't handle rejection for some reason.

It sounds like you've worked in some massively toxic teams. That's all I can say.
Post reply on HN