Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Building an Inclusive Code Review Culture

blog.plaid.com

Post reply on HN
Print view View original

41–50 of 81 posts

Re: Building an Inclusive Code Review Culture

#41

Post by btbuildem »

ghostbrainalpha wrote: post #38
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?

If everyone is thinking in the exact same pattern, you will have terrible monoculture, and a total inability to get past certain kinds of problem.

An effective team is comprised of people with a variety of backgrounds and approaches.

Re: Building an Inclusive Code Review Culture

#42

Post by akerl_ »

btbuildem wrote: post #41
ghostbrainalpha wrote: post #38

Earlier quoted context omitted.

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?

If everyone is thinking in the exact same pattern, you will have terrible monoculture, and a total inability to get past certain kinds of problem. An effective team is comprised of people with a variety of backgrounds and approaches.

This is a very uncharitable interpretation of the parent comment.

The parent comment appears to be referring to having people keep the shared style guide in mind when writing code, vs just letting the linter fix it. Extrapolating this to suggesting they don't want a team "with a variety of backgrounds and approaches" drags the conversation pretty far afield of that question.

Re: Building an Inclusive Code Review Culture

#43

Post by sqrt17 »

tdsamardzhiev wrote: post #32

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.

You sound like you didn't read the article. There's nothing about political correctness or try-hard inclusivity in that article but many (or indeed all) the things that common sense would dictate as a base of working code reviews.

Re: Building an Inclusive Code Review Culture

#45

Post by bluntfang »

Coding_Cat wrote: post #25
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…

Not all languages have good linting/autoformatting support (although luckilt they're rarer these days). For example: R.

This is a discussion to be had before coding starts on a project. Do you care about style? If yes, choose a language that cares about style.

Re: Building an Inclusive Code Review Culture

#46

Post by ergothus »

tdsamardzhiev wrote: post #32

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.

It seems overly simplistic to say that effective code reviews are inevitable if only you can prevent people from being rude. Certainly not a bad START, but I dare to think there may be additional ways a review can be a GOOD one as opposed to just not being a BAD one.

It's easy to forget that we're still learning how to write "good" code, and that many of our best practices continue to evolve, occasionally into VERY different things. A Code Review has to be able to identify poor practices as distinct from UNUSUAL practices, with that latter evaluated in terms of cost-benefits and not forgotten even when the costs are too high in the current project - that may be a practice to try out where there's less impact to being inconsistent. A Code Review has to balance the subjective opinions of multiple devs to try and achieve some consensus that is treated as objective. None of this is easy nor truly figured out, and not being a jerk is just one of multiple steps to accomplish.

Re: Building an Inclusive Code Review Culture

#47

Post by aequitas »

ghostbrainalpha wrote: post #38
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?

Depends on the kind of style 'subject' I think. If it is trivial indentation, bracket placement and other preference kind of things it probably wouldn't. But if the style things lean more towards design patterns, then often there is a proper reason behind choosing that style over another and it would help because you would notice doing it the 'wrong' way and you stop to evaluate and maybe spot a bug or design issue. I think that would fall in the category of code smells like when you use a framework in a convoluted way you're probably not using the framework right or over stretching it.

Re: Building an Inclusive Code Review Culture

#48

Post by fipple »

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.

In what language do those produce different results?

Re: Building an Inclusive Code Review Culture

#49

Post by aequitas »

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…

Btw, an added bonus of having a automation friendly standard for your codebase is that is can be easily converted in something you prefer reading. Prefer brackets below the condition instead of at the end? With one standard to convert to/from you can write a simple script to do that using a Git filter so every time you commit/checkout the transformation is automatically applied (https://git-scm.com/book/en/v2/Customizing-Git-Git-Attribute...).

Also one example on how to codify almost any style is using something like Sonarqube SSLR (https://docs.sonarqube.org/display/DEV/SSLR) where the code is converted into an abstract representation where comprehensible rule can be written for (instead of for example a bunch of regexes). But you really have to be deep into wanting to enforce every aspect of a style to dive into this one.

Re: Building an Inclusive Code Review Culture

#50

Post by com2kid »

bluntfang wrote: post #45
Coding_Cat wrote: post #25

Earlier quoted context omitted.

Not all languages have good linting/autoformatting support (although luckilt they're rarer these days). For example: R.

This is a discussion to be had before coding starts on a project. Do you care about style? If yes, choose a language that cares about style.

Not always possible if the requirements of the project dictate the language.
Post reply on HN