Building an Inclusive Code Review Culture • Hacker News Board

Re: Building an Inclusive Code Review Culture

#11

Post by notyourwork » Mon, Jul 30, 2018, 3:48 PM UTC

zhangela wrote: post #7

notyourwork wrote: post #5

Earlier quoted context omitted.

Not everyone is the right person for a specific code review. Even on small teams someone may be more of a subject matter expert in one facet of your system or code. Therefore load balancing isn’t really the right way to look at it. The idea should be to seek feedback inclusively and welcomed from all teammates. As a submitter your job is to make sure you get the right feedback if there is someone more knowledgeable o…

Agreed it shouldn’t be an even distribution of review requests across the board. That said, if a few people are getting all the reviews, it’s difficult for them to make progress on their project work, and for other engineers to learn code review best practices and get familiar with the codebase.

It’s a balance but you need to balance team priorities. If you aren’t shipping code because of your time on code reviews you need to better manage your time. Call it out during stand up (assuming your team has them) and make it clear you don’t have time or code reviews today because you need to focus on your deliverable. If the review is more important your team can identify this collectively. However, individuals should never feel you describe, if so it’s a lack of coordination across team.

Re: Building an Inclusive Code Review Culture

#12

Post by jelambs » Mon, Jul 30, 2018, 3:52 PM UTC

lolc wrote: post #9

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

Yes. There shouldn't be any reviewee except the code itself. But this is hard to get right. One of the questions I try to ask myself with any comment I write as a reviewer: "Would I accept this comment?".

Author here, I think that’s a great approach. We surface our guidelines for every PR by including a link to them in the description template. This acts as a reminder for people to be thoughtful about their review. I think the fact that our guidelines were based on input from the most senior to the most junior engineers says a lot about how much we value everyone’s opinions and perspective, not just the loudest voices.

Re: Building an Inclusive Code Review Culture

#13

Post by starbugs » Mon, Jul 30, 2018, 3:52 PM UTC

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

Not sure why this is being down voted. This is exactly the number one thing I have experienced happening with code reviews, especially peer reviews.

This attitude often completely breaks the effectiveness of the whole process, making it a deal breaker for quick iterations and agile projects.

Re: Building an Inclusive Code Review Culture

#14

Post by daveFNbuck » Mon, Jul 30, 2018, 3:53 PM UTC

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

> still have to find something to gripe about because I have 20 years on this kid

I wish I had someone with decades more experience than me taking the time to find areas where my code could improve. Regardless of their motivation, this sounds like an amazing resource that you're lucky to have.

Re: Building an Inclusive Code Review Culture

#15

Post by aequitas » Mon, Jul 30, 2018, 3:55 PM UTC

"If someone has committed many crimes against the style guide in a PR, the reviewer should point them to the style guide..."

Everything a computer can do trivially should not be done by humans. There are plenty of tools available to validate code to all the standards you can dream up. And most languages allow you to even automatically fix style errors or even enforce it for compilation (thank you Go).

So instead of codifying it in a style guide document, codify it in config files for linting and autoformat tools instead. The defaults of those tools are often fine for what you need and exceptions can as easily be documented in them as well.

Re: Building an Inclusive Code Review Culture

#16

Post by nilkn » Mon, Jul 30, 2018, 3:57 PM UTC

daveFNbuck wrote: post #14

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

> still have to find something to gripe about because I have 20 years on this kid I wish I had someone with decades more experience than me taking the time to find areas where my code could improve. Regardless of their motivation, this sounds like an amazing resource that you're lucky to have.

I think their motivation still matters quite a bit. Just because someone has 20+ years of experience doesn't mean they're actually leveraging any of that for their review. If they're nitpicking something unimportant and they're only doing it for political reasons, then in that act they are contributing negative value to the team, regardless of experience.

Re: Building an Inclusive Code Review Culture

#17

Post by spenrose » Mon, Jul 30, 2018, 3:57 PM UTC

Based on my experience with Mozilla's mature code review culture, this article is excellent. If I were going to make one suggestion, it would be to emphasize the points that author jelambs makes below: this approach to code review is inseparable from a commitment to deep shared understanding of the code base. It both depends on and supports that understanding. It's an investment in quality as embodied in a particular technical approach and body of work. It's not cheap, but what it buys you can be very valuable.

Re: Building an Inclusive Code Review Culture

#18

Post by mattlondon » Mon, Jul 30, 2018, 4:00 PM UTC

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

We dont have that problem where I work so it does exist. I've sort of experienced something similar myself when I've been doing reviews though - the feeling that you've got to find something to comment on. I've also been "star-struck" by getting reviewed by someone very senior or "famous" in the open source/tech world and basically blindly doing what they suggest.

The problem we do have though is huge queues for qualified reviewers and getting randomly allocated a reviewer somewhere on the other side of the planet so that even if they have minor comments (e.g. trivial typos in comments) you're looking at a 48 hour turnaround to get your code checked in sometimes.

I guess for the age/hierarchy thing, it could be possible to make the reviews double-blind via the tooling? I.e. reviewers dont know who they are reviewing, and the reviewee doesn't know who is reviewing them. Obviously once the code is committed then everything would become clear and transparent. Might help in larger orgs, although I dont doubt that it would be fairly easy to make an educated guess who is writing the code a lot of the time, and you'd be able to dig into git/whatever to see the details, but just hiding it in the review tool might go a long way to prevent unconscious biases. If someone has a real axe to grind is deliberately going out of their way to find out who wrote the code during a double-blind review so they can make special comments then you probably have a bigger problem.

Re: Building an Inclusive Code Review Culture

#19

Post by ISL » Mon, Jul 30, 2018, 4:05 PM UTC

aequitas wrote: post #15

"If someone has committed many crimes against the style guide in a PR, the reviewer should point them to the style guide..." Everything a computer can do trivially should not be done by humans. There are plenty of tools available to validate code to all the standards you can dream up. And most languages allow you to even automatically fix style errors or even enforce it for compilation (thank you Go). So instead of c…

> Everything a computer can do trivially should not be done by humans.

Agreed, to a point. In many contexts, it is important for humans to do things themselves occasionally in order to learn how, even if a computer is generally used to do those things far more quickly.

Re: Building an Inclusive Code Review Culture

#20

Post by sanderjd » Mon, Jul 30, 2018, 4:10 PM UTC

crunchlibrarian wrote: post #2

Most code reviews I've participated in have devolved into the senior/loudest person making sure that people know who they are and what everyone's place in the hierarchy is. Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just i…

This is one failure mode that definitely exists. Another failure mode is less experienced folks who believe all their code is perfect and take criticism personally.

I honestly don't know which failure mode is more common. I do know that I had the latter problem in spades when I was earlier in my career and that I'm really paranoid now about being that senior person on a power trip.

But if everyone is trying really hard to both act in good faith themselves and assume good faith in their coworkers, careful reviews are the best way I'm aware of for getting great code into the tree.