
Ask HN: How do you build a healthy code review culture? - pinkunicorn
I&#x27;m currently working at a startup that has a very bad code review culture. People either treat design&#x2F;coding-style comments as annoying or not respect them at all. Needless to say, the code base is beyond shitty and new developers are finding it tough to understand the logic and figure out why the code is doing what its doing. I started out by suggesting minor design changes, better abstraction into classes and functions, defining clear interfaces between different components, but most of the time no one respects or even responds to those comments which makes me paranoid that my comments are probably stupid.<p>How do I turn this around and build a healthy code review culture?
======
EliRivers
People get precious and personal about their code. They do. Some of them take
any critique very personally. Some of them have a tantrum, some of them just
ignore anything anyone says. Programming ego is a real problem.

A couple of jobs ago, I worked somewhere with an excellent code review
culture, and it began with a simple, written standard, very small, that
everyone had agreed to. I now refuse to review code unless I am able to say
"This code doesn't conform to the document you agreed to; it's not that _I_
think this is bad code, it's that it doesn't meet the document that you agreed
to." If there's no document to point at, no objective standard to meet or not
meet, I'm not reviewing it because it's just not worth the tantrums and
screaming from precious programmers.

There also need to be consequences for code that doesn't meet the document;
basically, it's not ready to merge/commit until it does. There needs to be a
way to allow code that violates the standard to be waived and permitted, with
agreement from the developer and reviewer, with the waiver recorded so the
developer feels listened to and the reviewer feels protected.

You're going to have to get buy-in for this from someone with power and
authority.

~~~
seanwilson
"If there's no document to point at, no objective standard to meet or not
meet, I'm not reviewing it because it's just not worth the tantrums and
screaming from precious programmers."

What about rules for less objective things like how readable a function name
is though? If someone wants to be difficult addressing code reviews, there's
always plenty of subjective feedback they can do this for.

~~~
dmm
The Google c++ styleguide[0] has some examples, in general and specifically
for files, functions, etc. Looking over them most are pretty objective so
maybe it's worthwhile to come up with some usable rules that enforce qualities
you notice readable code shares, even if they aren't completely comprehensive.

[0]
[https://google.github.io/styleguide/cppguide.html#Naming](https://google.github.io/styleguide/cppguide.html#Naming)

------
bzalasky
Code reviews shouldn't dwell on style issues that can be handled by linting.
To the extent that you can automate some of the review process, and prevent
sloppy commits from being merged in by having them fail CI, you'll eliminate
part of the problem. Another thing that helps is reducing the semantic
distance between what a commit message says a commit does, and what it
actually does. This is pretty easy to enforce relative to more subjective
aspects of coding. The last thing that can make a big difference is not
getting hung up on wanting to rewrite a commit as you would have written it,
but focusing on improving understanding and clarity for whoever will inherit
the code next. If there are serious issues with the architecture and patterns
the code is using, CR can be too late in the process to address them
effectively (depends on the scale involved). I'd try to collaborate with other
engineers earlier in the process to help set them on the right path.

------
seren
I am working in a team where code review are mandatory, but this was not so
hard to implement because they are part of regulatory requirements.

Your culture change is a problem of change management[0]

There are different model, but some example of actions, are :

* finding a sponsor

* creating a common vision of the end result and benefits, communicate it

* identify early adopters to deploy it with them

* identify people resistant to change (and why ? Is there a way to convince them ? [1])

* communicate your early success

Most of it is basically common sense, but it is good to have a kind of check
list to help you establish your plan.

One thing that is important, because it is otherwise frustrating, you have to
lay out very clearly what are the "coding rules" and what the code reviewer
will actually check up front. If the "contract" is clear, it is easier to
follow it.

The most annoying situation is written a whole bunch of code and having to
rewrite everything.

[0]
[https://en.wikipedia.org/wiki/Change_management](https://en.wikipedia.org/wiki/Change_management)

[1] for example, a human tendency is to resist change because a fear of loss
of power. So using that you could decide that the guy that is more resistant
to change will actually be the one responsible for doing a fair share of code
review.

------
pmiller2
We do a combination of things already mentioned here at work.

First, we have git hooks that prevent code that doesn't pass certain standards
from even being committed. Mostly, that's stuff like PEP8 and eslint, but
there are a few other automated checks.

Second, we require at least one other person to review and leave a comment
that says the code is acceptable to merge. Anybody can look at anybody else's
pull request, and anybody else can block a pull request from being merged if
they feel it is necessary. These things are also enforced by automated tools.

Finally, we have CI tools that not only run our extensive test suite (which
must pass before a PR can be merged), but also re-run the linting checks.

I've never seen anyone get defensive about a comment on their PR in this
environment. The result isn't perfect code (there are still some hairy spots
in our code, but when one has 300K+ LOC, one expects that), but it's rarely
very bad, and that's the point.

------
kat
I worked at a company that started having code reviews. We started off by just
"presenting" code to the team. The team would point out any thing that looked
like a bug or areas that required extra QA effort. Just knowing you would have
to present your code was enough to increase code quality. We didn't worry
about style/best practices/subjective feedback at the beginning. Over a few
months we all became more comfortable receiving and giving feedback and that's
when we started to solidify the style guidelines. Also our ability to catch
bugs without QA cycles helped increase buy-in for code reviews.

------
panic
Does management understand the benefits of a healthy code review culture?
Managers often support code review in the abstract, but when faced with an
actual decision between respecting code review or shipping a feature faster,
they'll choose to ship faster. Your colleagues are probably just responding to
this.

------
eecks
I kinda like reading stuff like this online. It reminds me that I (and my
colleagues) are doing this stuff extremely well.

The only way to change your culture is to draw up standards and then fail any
code review that does not meet the standards. Nothing goes live until it
passes code review.

------
thisone
if you need to get heavy handed about it, people are probably going to leave.
Which may be a good thing, since you shouldn't have to get heavy handed about
simple things like styling.

I'm a big fan of strong linting where failure to pass linting will fail the
build.

The general reason why I feel that way isn't anything to do with readability
but because linting can teach you what NOT to do and make you a better
programmer.

Code reviews shouldn't be "move this comma" let linting handle that.

Code reviews can then be more "is what this is doing sensible, are the test
cases covering the change, and (possibly most importantly) do I understand how
this changes the application"

------
deeteecee
at this point, you have to ask that specific question directly to either the
team, the leader, or management and ask them why they don't look at the
comments.

------
TerryADavis
When I was a young programmer, all the code I looked at was awful. If there
was a different format indentation, that distracted me. All I could think
about was code format and superficials.

I worked at Ticketamster 1990-1996.

I went back in 2003. I remembered my first assignment. In 1990, the Linker at
Ticketmaster that Troy wrote ran out of room because the MAP file grew too
big.

In 2003, I went back a found a terrible kludge that I did in 1990 because I
was stupid. In 2003, I fixed it correctly. I looked at the code and thought
how beautiful it was. In 1990, I thought the code was bad.

As you get more experience, it is a little easier to read code. Take my word
that all code is bad, LOL.

Personally, I don't like code that is made of 5-line functions with no rhyme
nor reason spread seven levels deep.

Somebody said the smaller your functions the better. Bad advice.

God says... helices mists gewgaw's specced mesdames Lyndon's favoritism's
fable's unequal apportions radioactivity's besoms thicknesses cordoned towel's
spryer bobcat's suppressed Petra windscreen neckline overproduce meridians
cartons prophylaxis's shoestring's retrospection's flubs amen thatcher
bombshells cantankerous

