6 ms·

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be val

The more experienced I get the more I want to work on teams with a higher degree of trust than ones like this. Don't get me wrong I think code review can be valuable but on high preforming teams it works best on a as needed basis. The way this has worked successfully for me in the past is as a new joiner to a team you submit for code review every time. My first couple of reviews tend to have a good bit of feedback as I settle in to the style of the team and often a new language or framework. After a while though I become a domain expert for the projects I am working on, and code reviews end up becoming a rubber stamp. At that point it's just something slowing me down. I'd request review when I need it, or I'm making a big change that will effect everyone, otherwise there are many cases where there is really only one way to skin this cat and it's totally perfunctory. This sort of arrangement also requires a bit of ownership, if you see reports floating around about an endpoint you just worked on having issues you have to be able to be trusted to jump on it. I feel like trust and ownership are touchy feely people problems so tech people invented code reviews and linters to try to make up for not cultivating the sort of culture where they aren't required on every single little thing.

edgyquant 5y ago
On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.
Cerium 5y ago
I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices. When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch. I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.
Kranar 5y ago
I feel I work on a high performance team and code reviews are a joy. When I submit my code for review it's an opportunity to share my work with my peers, potentially explain my thought process and my work process and we get to learn from one another, and when I get code to review it's the opposite, I get to see how my coworkers think and tackle problems. I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again. Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.
the_jeremy 5y ago
I trust the people whose code I review to be competent developers. But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Even if the creator is the expert and the reviewer is there to catch only obvious mistakes, that still has value.
- bavila 5y ago
  > But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Case in point: OP used the word "effect" when he should have used "affect". Just like pudding, there's always room for a code review.
bmj 5y ago
On my team, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review. I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."
- larusso 5y ago
  I‘m called a principal engineer in my organization (I know that can mean everything or nothing) I like to use code review requests and sometimes gates also to put a fence in front of my ego. Also this is a great way of sharing knowledge. I had great sessions where questions about X and then answers verbally or in text suddenly put stuff into different perspective. Sometimes I realize that my solution might be to complicated or an more generic, performan, (paste fancy adjective to describe your code here) is easy to achieve.
digisign 5y ago
Yes on trust. However… I'm pretty darn experienced in what I do these days, and every day I still write a bit of crappy code, usually as I come to an understanding over time of the current problem. WTFs come from code written six months ago as well as yesterday. If someone can point out an issue early, it saves everyone time. So, I welcome it.
quadrifoliate 5y ago
The popularity this sentiment on HN terrifies me. Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does. If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.
- ryandvm 5y ago
  I disagree. If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution. Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area. It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.
  - AnimalMuppet 5y ago
    If your code reviews aren't catching bugs, the correct answer is to fix the code reviews, not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)
hsn915 5y ago
This used to be common sense in the few companies I worked at around 2012. People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future. But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously. It's one of the reasons I don't want to work at any programming company anymore. The really good programmers I know don't look positively at code reviews. If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.
- nradov 5y ago
  Where do we find all of those better programmers?
hinkley 5y ago
There's a flip between when the feedback is super, super helpful (consensus building, as you say), to a moderate inconvenience to screwing up your estimates because you seem to always post a PR right after the reviewers have started some new thing. Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests. It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.) A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.
smokey_circles 5y ago
I see 2 clear problems with this ideology: 1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success. 2) Your team will grow. You will need to share this knowledge somehow before the trust can be given But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about. I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen. I trust mistakes they've made are for the same reasons. What you're trusting is your teammates are top tier 1% every time and bordering on being super human. I'm trusting that my team has my back and we're in it together.
kkirsche 5y ago
This is scary to read. We use code reviews for changes and we’ve found that our more junior team members can bring a point of view our more senior team members can’t and vice versa. It’s one of the best parts of working in code for me. With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical. Code reviews are a “we” activity not a “your” and “my” activity.

edgyquant 5y ago

On my team we have one other engineer, no matter your or their official position, just do a quick glance over for obvious issues. This does not prevent real bugs from getting merged in, the test cases are supposedly for that but even those have some slippage.

Cerium 5y ago

I work on a team that runs following a high trust and domain ownership model. When I joined our process was "write some code, test it, be confident in the code, commit it to trunk". A number of years ago we worked our way through "Clean Code" in a reading group and decided to try out code reviews. Each week one of us would present our recent work and the team would tear it apart line by line, and in the process we developed a better understanding of our group philosophy around style and practices. When we switched to Git we decided that we liked code reviews and wanted to make them part of merging to develop. Depending on the content the reviews are deeper or more superficial. Many times even when I review the code of somebody making changes in their own subject matter domain I have found bugs that simply needed another set of eyes to catch. I like code reviews and in the context I use them find that they are a complement to trust, not a substitute.

Kranar 5y ago

I feel I work on a high performance team and code reviews are a joy. When I submit my code for review it's an opportunity to share my work with my peers, potentially explain my thought process and my work process and we get to learn from one another, and when I get code to review it's the opposite, I get to see how my coworkers think and tackle problems. I have never once felt like a code review did anything to erode trust and I don't see how a code review could slow down a work process. Using git, as soon as I submit code for review I begin work on the next task and if I need anything from the branch I submitted for review, I just merge that into the new branch. If changes are requested I make the changes and then merge again. Code reviews are among the best ways of ensuring that code remains consistent and that everyone is learning from everyone else. That's why in my team all pull requests are reviewed by two people including one junior developer.

the_jeremy 5y ago

I trust the people whose code I review to be competent developers. But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Even if the creator is the expert and the reviewer is there to catch only obvious mistakes, that still has value.

bavila 5y ago
> But everyone occasionally makes typos, misreads requirements, has blind spots, etc. Case in point: OP used the word "effect" when he should have used "affect". Just like pudding, there's always room for a code review.

bmj 5y ago

On my team, the level of scrutiny a merge request receives depends on the author and the functionality. MRs from a principal or lead engineer tend to get rubber-stamped at a higher rate, though, to your point, many of us will explicitly call out code that we would like to have reviewed in more detail. If a more junior level developer submits a merge request, it is highly likely their code will get a close review. I'm not sure I agree with your premise that code reviews are the product of a culture with a lack of trust and ownership. I think, in fact, submitting to a review process is a sign of ownership -- it's saying "hey, the quality of the code we produce as a team is important to me, and I know I make mistakes."

larusso 5y ago
I‘m called a principal engineer in my organization (I know that can mean everything or nothing) I like to use code review requests and sometimes gates also to put a fence in front of my ego. Also this is a great way of sharing knowledge. I had great sessions where questions about X and then answers verbally or in text suddenly put stuff into different perspective. Sometimes I realize that my solution might be to complicated or an more generic, performan, (paste fancy adjective to describe your code here) is easy to achieve.

digisign 5y ago

Yes on trust. However… I'm pretty darn experienced in what I do these days, and every day I still write a bit of crappy code, usually as I come to an understanding over time of the current problem. WTFs come from code written six months ago as well as yesterday. If someone can point out an issue early, it saves everyone time. So, I welcome it.

quadrifoliate 5y ago

The popularity this sentiment on HN terrifies me. Please don't do this. I keep finding rubber stamp code reviews (or none at all) while diving into codebases for where bugs were introduced, and 9 times out of 10, it's someone who didn't bother to get proper reviews for their code because "it's just something slowing me down". I can assure you that you have bugs and usability issues in your code, not just "linter issues". Everyone does. If your code reviews are becoming rubber stamps, that's an indication that you need to slow down and teach other team members about the system, or hire smarter people who can understand your reviews more quickly. Don't respond by getting rid of the review process as a knee-jerk response to these sorts of organizational constraints.

ryandvm 5y ago
I disagree. If you have a practice that isn't effective (e.g. code reviews that aren't preventing bugs), doubling-down is almost never the correct solution. Code reviews are like unit tests; they're helpful under the right conditions. Mostly on the tricky bits or when a dev is working in an unfamiliar area. It's a common fallacy for folks to believe that if a little of something is good, then a lot of it must be better. I've been at places with 100% code test coverage or requiring EVERY SINGLE PULL REQUEST to be reviewed. It's obnoxious, and it doesn't do shit to prevent bugs. Instead it causes folks to burn out on the practice and write crap unit tests or stamp "LGTM" on every PR they're forced to evaluate. People have a limited capacity for cognitive engagement and wasting that on piddly crap means they can't spend it on the important bits.
- AnimalMuppet 5y ago
  If your code reviews aren't catching bugs, the correct answer is to fix the code reviews, not to stop doing them. I mean, it's possible that code reviews are worthless. But enough people have found value in them, across enough organizations, enough code styles, and enough decades, that it seems really hard to conclude that. It seems more probable that the team that finds them useless is doing them wrong. (Yeah, I know, No True Scotsman. Still, the first response should be "maybe our team is doing them wrong, and can we fix that?", rather than "let's stop doing them".)

hsn915 5y ago

This used to be common sense in the few companies I worked at around 2012. People just pushed code. We occasionally held some review sessions to go over code and exchange comments / ideas about what might be wrong with this or that code and how to prevent this kind of mistake in the future. But starting from around 2016, I could never ever find a company that does not have mandetory code reviews for every single commit, and this bothers me tremendously. It's one of the reasons I don't want to work at any programming company anymore. The really good programmers I know don't look positively at code reviews. If you need code reviews, ask for it. If not having mandetory reviews results in a huge stream of bugs, maybe you should hire better programmers instead of cheap ones.

nradov 5y ago
Where do we find all of those better programmers?

hinkley 5y ago

There's a flip between when the feedback is super, super helpful (consensus building, as you say), to a moderate inconvenience to screwing up your estimates because you seem to always post a PR right after the reviewers have started some new thing. Then farther down that continuum someone is busting your chops for some stupid reason that, after you get back from lunch or coffee, you realize is completely valid (or even gives you an idea for an enhancement) and okay you change how a conditional works or add some more tests. It makes me wonder if in a corporate setting PRs would be better treated as pseudorandom audits, based on a random factor and a weight for automated linting (so people don't learn to game the linter, which can be some of the worst code you ever see.) A related issue is that most tools make it pretty difficult to look at and interact with old PRs. Feedback loops have some slop, and while it's better that you get the feedback as you are writing the code, fast follow often works as well, and I think we'd be better as a group if you could request revisions on a recently landed PR after the fact. This is mostly okay but you really need to fix your variable names, conditional block, and you missed a boundary condition over here that might not break today, but there are already stories in the backlog that will break it for sure.

smokey_circles 5y ago

I see 2 clear problems with this ideology: 1) There is too much knowledge to fit inside one person's head. Cooperation is the keystone of success. 2) Your team will grow. You will need to share this knowledge somehow before the trust can be given But in defense of this view point: code reviews shouldn't be the opposite of what you're talking about. I trust my teammates to catch any mistakes I've made. I'm human and therefore fallible. It's going to happen. I trust mistakes they've made are for the same reasons. What you're trusting is your teammates are top tier 1% every time and bordering on being super human. I'm trusting that my team has my back and we're in it together.

kkirsche 5y ago

This is scary to read. We use code reviews for changes and we’ve found that our more junior team members can bring a point of view our more senior team members can’t and vice versa. It’s one of the best parts of working in code for me. With that said, code reviews only work, in my experience, if team members are strong communicators. Understanding each other, empathizing with issues, and being collaborative rather than tearing people down is critical. Code reviews are a “we” activity not a “your” and “my” activity.