Building an Inclusive Code Review Culture • Hacker News Board

Re: Building an Inclusive Code Review Culture

#31

Post by aequitas » Mon, Jul 30, 2018, 4:43 PM UTC

jelambs wrote: post #22

ISL wrote: post #19

Earlier quoted context omitted.

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

Agreed, computers are generally way better than humans at catching many style issues. We utilize linters extensively, those enforce many of our style guide rules. However, there are some, such as the example pictured in the article, that aren’t always detectable with a linter. In these cases, we use the style guide as the source of truth to reduce the amount of personal preference that comes into code reviews.

Agree, for everything that doesn't go automatically or would be to much effort to automate a single source of truth must be maintained.

Re: Building an Inclusive Code Review Culture

#32

Post by tdsamardzhiev » Mon, Jul 30, 2018, 4:44 PM UTC

The real problem here are people with bad attitude and poor communication skills. Bullying other people is a punishable offense in real life, and I don't see why code reviews should be any different. Have some basic rules regarding communication. Issue a public warning for first-time offenders. Give them the boot if they do it again. There, problem solved.

Re: Building an Inclusive Code Review Culture

#33

Post by megaman22 » Mon, Jul 30, 2018, 4:46 PM UTC

starbugs wrote: post #29

daveFNbuck wrote: post #23

Earlier quoted context omitted.

If someone is that much more experienced than you are, it can be hard for you to tell the difference between nitpicking and something that experience has taught them is more important than you realize. Even if it is just nitpicking, that still means your code has nits to pick.

Be sure that I can come up with nitpicking on just about anything that you write, regardless how brilliant you think your piece of code is. This just costs money, burns people, and results in a bad culture. But I am sure as long as you're not the one who is in the weaker position that's totally ok for you, because you give considerably more f ks about yourself than about your company's success.

Unless code review is a rubber-stamping process (in which case why bother? just party on production...), you're inviting criticism and improvement, so freaking out about receiving some and getting defensive or calling people toxic is... less than productive.

One prolly shouldn't be a jackass as a reviewer, but if comments or questions on what you're doing get your back up, one should perhaps grow a thicker skin as a reviewee.

Re: Building an Inclusive Code Review Culture

#34

Post by ataturk » Mon, Jul 30, 2018, 4:46 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…

One reason I left my previous role was because the code reviews were miserable and the team refused to automate any style matters which would have externalized and automated away many issues completely.

The team claimed they were highly collaborative, but in reality, it was two heads down guys who didn't want to be bothered and would just offer niggle after niggle that had little bearing on overall quality, making the code more maintainable or ship-able. I realized they acted this way mainly because of personality deficits and likely office politics and as a cynical lever of control. There was a third developer who was genuinely decent and considerate and who had good insights to share and who was genuinely collaborative. Lastly, there was a newbie whose reviews were valuable for him to do but added no real value for me.

I have worked in large and small organizations, been on projects that generated billions in revenue, and never went through what I went through there. I realized I wasn't doing my best work anymore just because I loathed the process and it wasn't going to change. Being an asshole, gas lighting, and making up policies on the fly does not help the bottom line of a company, it just pisses people off for no good reason. But hey, they are free to write the code exactly how they want and I got a $10K raise. A gift for all of us!

Re: Building an Inclusive Code Review Culture

#35

Post by mnarayan01 » Mon, Jul 30, 2018, 4:50 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…

  if a = foo()

versus:

  if (a = foo())

Sometimes idiomatic patterns vary based on things which are not amiable to mechanical correction.

Re: Building an Inclusive Code Review Culture

#36

Post by munchbunny » Mon, Jul 30, 2018, 4:54 PM UTC

mnarayan01 wrote: post #35

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…

if a = foo() versus: if (a = foo()) Sometimes idiomatic patterns vary based on things which are not amiable to mechanical correction.

While you have a point, I don't think the exception should disprove the rule. Even if a few specific style patterns can't be automated, the bulk can be and it's probably better to have them enforced at the pre-compile stage.

Re: Building an Inclusive Code Review Culture

#37

Post by quxbar » Mon, Jul 30, 2018, 4:58 PM UTC

mnarayan01 wrote: post #35

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…

if a = foo() versus: if (a = foo()) Sometimes idiomatic patterns vary based on things which are not amiable to mechanical correction.

The trick is to ban even the correct but (to a human) ambiguous patterns. In this case, I wouldn't let you do assignment on the same line as the conditional.

Re: Building an Inclusive Code Review Culture

#38

Post by ghostbrainalpha » Mon, Jul 30, 2018, 5:06 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…

Noobie question.....

Do you think there is any value in writing the code with the correct style before it is fixed in the compiler?

Like is it easier to help out and spot a problem quicker, if everyone is thinking in the exact same patterns when they write code?

Re: Building an Inclusive Code Review Culture

#39

Post by majormajor » Mon, Jul 30, 2018, 5:07 PM UTC

daveFNbuck wrote: post #23

nilkn wrote: post #16

Earlier quoted context omitted.

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.

If someone is that much more experienced than you are, it can be hard for you to tell the difference between nitpicking and something that experience has taught them is more important than you realize. Even if it is just nitpicking, that still means your code has nits to pick.

I don't believe this. Every time I've seen subtle problems that experience taught me were more important than first appeared, I've been able to explain why. And so I put that explanation in the review.

Re: Building an Inclusive Code Review Culture

#40

Post by nilkn » Mon, Jul 30, 2018, 5:11 PM UTC

daveFNbuck wrote: post #23

nilkn wrote: post #16

Earlier quoted context omitted.

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.

If someone is that much more experienced than you are, it can be hard for you to tell the difference between nitpicking and something that experience has taught them is more important than you realize. Even if it is just nitpicking, that still means your code has nits to pick.

Code reviews are generally public and visible to the entire team and often even to other teams. While it's true that the reviewee may not be able to distinguish the two cases being considered, there may be other developers on the team or in the company who witness the review and can make the distinction. In fact, this can cause the "toxic reviewing" practice to spread to other experienced senior developers, who may see others with similar positions doing the same thing and then feel they must do it too in order to signal that they are, in fact, senior.