Live data from Hacker News

No code reviews by default

raycast.com

111–120 of 315 posts

Re: No code reviews by default

#111

Earlier quoted context omitted.

If it catches ANY bugs it is objectively better than no PR/code-review from a bug standpoint. Unless you're arguing that pr/code-review creates more bugs than it solves? Again, I think the question is value. Is a dev's time best used in code-review vs. something else? It's almost certainly the case that time is better spent elsewhere depending on the develop and needs or the organization.

> If it catches ANY bugs it is objectively better than no PR/code-review from a bug standpoint. No, that's false. That's only true if you also assume that people write the same quality code whether there is a code review process or not. Which might or might not be true, no idea.

if anything I would think that person would write same code but without PR would lost the opportunity of having better quality. People are blind to own mistakes. I can't count how many times I would stare at a code not seeing something and other person would spot problem almost immediately.

Re: No code reviews by default

#112
post #102

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

> 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 a people problem that should be fixed.

You say that like fixing people is an easy thing to do. It's the hardest thing to do. And the fact I've seen this at so many companies/teams leads me to believe that it's an extremely common thing.

> when you look at a code review and you see problems in what's there, catching them in the code review is great, because that means less chance of having to fix it later. > if you see a code review and nothing's wrong with it, you just approve it. sometimes you can note things as "nits", meaning, fine for now but maybe next time do it this way.

Yea... Those aren't getting done at 9/10 companies. They're getting ignored.

> syntactical style and formatting should be automated, if not with tooling that actually rearranges the code in that way (e.g. [black](https://github.com/psf/black) for Python) then at least including well tuned linter rules. We use black, my own zimports tool for sorting imports, flake8 with four or five plugins as well as "black check", so we spend exactly zero time dealing with anything to do with style or code formatting, including having to even type any of it. if people are fighting over code style then there need to be tools that lock all of that down so there's no disagreements.

Yea, we have those, I still get asked for stupid ass code style changes at every company for the last 8-years. Why? Because people can spot them and understand them. But can't code review for shit. They can't understand the idioms for the programming language, they can't understand how certain paradims work, they don't pay attention to root causes of bugs, they don't know how to design a database, they don't pay attention to performance, etc. I would say 80% of the industry can't code review for crap. And that is the biggest problem and because they can't do that all the other problems are just symptons.

> I work on the Openstack project and that is where I learned to do code review with Gerrit, it is completely essential for large projects and is an enormous productivity enhancement. Decades ago before CVS was even invented, the notion of using source control seemed crazy to me. That position was obviously insane, even though we were stuck using Visual Source Safe at the time. That's how it feels to me to be doing a real project today without code review and formatting tooling in place. Like even for myself, if working alone, for a large project with lots of changes I still use a code review tool, just to review myself.

This doesn't dispute or refute my point. It merely says in some cases it's used well. It doesn't remove the fact that majority of the time it's cargo cult and the majority of people who code review can't code review for crap or the fact that a high percentage of people just skim code review.

Re: No code reviews by default

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

Absolutely. I take issue with all 3 of his main points. You covered the first point well, so, I'll take the last 2.

> Pull requests don't prevent bugs. Reviewing code is hard! You can catch obvious mistakes, but you won't have consistent in-depth scrutiny. Many issues reveal themselves only after extensive use, which doesn't happen during code reviews. We rely on fast iterations with user feedback.

So many things I can say about this. "Fast iterations with user feedback" are great, until you introduce a security bug that you find out about because someone exploits it. Catching "obvious" mistakes alone has significant value. Since he acknowledges that code reviews catch some bugs, this criticism seems to be that they don't catch all bugs. To that, I say: show me a practice which does catch all bugs, and I'll show you why it doesn't. Even the most rigorous development process doesn't prevent all bugs. [0] This just doesn't sound like a fair criticism to me.

> Pull requests slow down. Code reviews aren't high priority for engineers. They have to make time to clean up their review queue. In reality, PRs are an afterthought and not what you want to spend your time on as an engineer.

This is largely a culture problem. Code review should be a high priority for engineers. Maybe it's not the highest priority, but, it's fair to expect one could get a decent review on a PR that isn't too large in, say, 1-2 days or less. If you have PRs sitting in review for weeks at a time on a regular basis[1], one of two things is happening: either engineers aren't properly prioritizing code review, or your PRs are too large and/or have too many dependencies.

Which brings me to the second thing you can do, which is make PRs that are as small and simple as possible, but no more. Minimize dependencies to help make your PRs understandable. And, it should be fairly obvious, but the fewer lines of code (including config changes) you have in your PR, the easier it will be to review, so, the faster it will get done.

Code review is also a great opportunity to learn and teach. Every senior+ engineer should be reviewing code frequently, IMO.

Finally, yes, code reviews will slow you down if your basis for comparison is just "engineers merge code when they think it's ready." That's intentional, and good. The cost is more than paid for by better code quality, leading to fewer instances of "here's a PR to fix the issues with my previous PR."

---

[0]: https://news.ycombinator.com/item?id=29801451

[1]: I've only had this happen once or twice in my career so far. In each case, it was because the code surrounding the code I was working on was not stable and kept changing. These were clearly exceptional instances.

Re: No code reviews by default

#114

I think I largely agree with this article. When I worked for Apple, we had a strict "all code needs to be reviewed" policy, which I had no problem with. Then, after being there for about 1.5 years, I make a PR, ask two different people to approve it, which they do, and it gets merged. A week later, the code is released, it looks like my code caused a fairly major bug. My manager's manager and my manager had a meeting…

I don't know what your point is. You would have gotten in trouble whether or not code reviews were present. If you're saying that code reviews should not be default, then the entire blame would rest on your shoulders anyway. Are you saying you wish you got in less trouble at Apple and the code reviewers should have gotten in more trouble for missing YOUR bug?

Re: No code reviews by default

#115
I used to love code reviews when I worked for Motorola in 2004, it was doing wonders, we observed showing up "phantom developer" where mix of inputs from different people was causing new look on code. Those were Fagan-like code inspections. A lot of paperwork (we were doing formal reviews on paper, with one person in role of reader), but it was working. This whole paperwork was forcing people to follow rules and it seemed to work (but was sometimes very painful). Latter in other companies I found out that code reviews are in most cases bullshit placeholders, and in many cases are only for making some developers feeling more important, or other devs not loosing contact with code. I saw most stupid things in code moved to position of examples how to write code only because of code reviews... Good code reviews are doing wonders, but as I can see without very strong culture in dev teams and whole company those code reviews don't add anything useful. And this strong culture is difficult. From observations it seems that this is easier to build in team culture to work with "master/main only" without branches and code reviews, than to build culture of code reviews which are working. From my observations most of comments in code reviews are variations on "I would code it in different way", or "why you are doing it in this way/I don't like your variable name" of course usually written in less direct way. Last year problems with Log4J2 showed that even in OpenSource peer reviews don't help. Good developers let to introduce to one of most used libraries something what never should be introduced there. For sure variable names were nice, but somehow whole "why we are adding this" was lost, because devs were looking only for some easy to spot things. So.... yep, good code reviews are important, but my guess is that most of code reviews are only to make some devs happier that they still know what happens in code, and that variables are named in acceptable by them ways....

Re: No code reviews by default

#116
post #84
post #70

Earlier quoted context omitted.

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

That is one of the things I do to learn new codebases.

Re: No code reviews by default

#117
post #84
post #70

Earlier quoted context omitted.

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

Sure. If they are good and have a decent grasp of using version control then will look at the history of a certain piece of code if they don't understand why it is the way it is. I do it often, even when I'm no longer a new hire.

Re: No code reviews by default

#118

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

It sounds like your company has a shitty culture. I've never had issues with code reviews in the companies I worked at. One company was particularly rigorous, where we had to print it ou on paper, and a room of engineers would go over things line by line. But that also had the best engineered software I had seen in my career.

Any issues of style should be in the style guide. Anything not in the style guide can be ignored. Or the style guide should be updated.

There should be a distinction between opinion and technical problems. If you suggest a change that is opinion-based, then it can be taken or ignored. I never comment on suggestions, because it's a waste of my time and theirs. If you are suggesting a chance that is a technical problem, then that should be changed. If the person doesn't want to make the change for the technical problem, then that's a culture problem in the team or company.

Re: No code reviews by default

#119
post #84
post #70

Earlier quoted context omitted.

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

Not necessarily looking through PRs, but presumably they need to work with actual code in your code base that was merged at some point, unless your product is growing so fast that new people just write new code all the time? (Same applies for not-new coworkers that now need to touch that code area anyways)

Re: No code reviews by default

#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 instead new problems and bugs are introduced while implementing their nits. All surrounded by a lot of noise that has to be dealt with in an already stressful multitasking environment. I agree with other posters in this thread that most people can't code review for crap and have very perverted idea about code review aims and goals. Code reviews as they are practiced now only serve to destroy trust, velocity and team morale. As someone noted elsewhere in this thread, if it's me (directly or indirectly) who's going to be held accountable for a deficiency in my code, whether it has been reviewed or not, why do I have to adjust my code to someone's arbitrary criticism? Common sense tells me that not requiring to have my code reviewed at all times and, by doing so, stressing my personal responsibility for the things I do, would actually increase my sense of ownership and keep me on my toes, more likely leading to better coded/tested changes, wouldn't it? I.e. it is not about blindly trusting all code that I produce but rather trusting in my ability to judge and make final decisions: to request code reviews or not, to address comments or not, to do my job as I see fit or not.
Post reply on HN