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.
No code reviews by default
111–120 of 315 posts
Re: No code reviews by default
#112> 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 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
#113He 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…
> 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
#114I 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…
Re: No code reviews by default
#115Re: No code reviews by default
#116Earlier 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?
Re: No code reviews by default
#117Earlier 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?
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…
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
#119Earlier 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?