4 ms·

> 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

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

zzzeek 5y ago
> 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 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. 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. > 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. 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. > 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- 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 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. > 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. 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.
- that_guy_iain 5y ago
  > 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 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.
curious_cat_163 5y ago
> I think the biggest problem with code reviews is it often becomes adversarial and people want to come out on top. Is that really a problem with code reviews? Or, is that a cultural issue for a given team?
baskethead 5y ago
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.
- autarch 5y ago
  > 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. I'd say any issues of style should be caught by required code formatting and linting tools, which optionally run pre-commit and _must_ pass in CI. This should all be handled _before_ code review. Then review can focus on substance, not style.
- throwawaythekey 5y ago
  > 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. I find that there are two components of style: - Formatting - Clean abstractions, modularization... everything else that goes into making you a great (not just passable) programmer Formatting should be handled by tools like others have said. For everything else I have the options: - Turn the style guide into a book which no one will ready anyway - Ask them to read an actual book but it isn't practical in the scope of a PR, and even after reading the ideas take a while to digest - Attempt to mentor the employee via the PR, often appearing pedantic because it's hard for a novice to appreciate what they don't know. - Code that passes CI is working code - screw everything else! I take the mentoring route whenever it's presented to me but have generally found that some other team members (especially as they get older) have no interest in best practice and become annoyed by the code review process. If the mentee has persistence it tends to work out but moreso over 6mth-1yr timeframes. For mentees without the patience we drop the code review and keep them in areas of the code base where they can't do too much damage. I guess what I'm trying to say is that our culture is willing to bend the processes to fit individual's strengths, and overall our workplace is pretty happy/chill. Maybe the engineering quality takes a hit but I can definitely sympathize with where the OP is coming from.

zzzeek 5y ago

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

that_guy_iain 5y ago
> 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 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.

curious_cat_163 5y ago

> I think the biggest problem with code reviews is it often becomes adversarial and people want to come out on top. Is that really a problem with code reviews? Or, is that a cultural issue for a given team?

baskethead 5y ago

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.

autarch 5y ago
> 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. I'd say any issues of style should be caught by required code formatting and linting tools, which optionally run pre-commit and _must_ pass in CI. This should all be handled _before_ code review. Then review can focus on substance, not style.
throwawaythekey 5y ago
> 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. I find that there are two components of style: - Formatting - Clean abstractions, modularization... everything else that goes into making you a great (not just passable) programmer Formatting should be handled by tools like others have said. For everything else I have the options: - Turn the style guide into a book which no one will ready anyway - Ask them to read an actual book but it isn't practical in the scope of a PR, and even after reading the ideas take a while to digest - Attempt to mentor the employee via the PR, often appearing pedantic because it's hard for a novice to appreciate what they don't know. - Code that passes CI is working code - screw everything else! I take the mentoring route whenever it's presented to me but have generally found that some other team members (especially as they get older) have no interest in best practice and become annoyed by the code review process. If the mentee has persistence it tends to work out but moreso over 6mth-1yr timeframes. For mentees without the patience we drop the code review and keep them in areas of the code base where they can't do too much damage. I guess what I'm trying to say is that our culture is willing to bend the processes to fit individual's strengths, and overall our workplace is pretty happy/chill. Maybe the engineering quality takes a hit but I can definitely sympathize with where the OP is coming from.