Ask HN: How do you build a healthy code review culture?

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.

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.

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

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

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.

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.

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.

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.

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"

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.

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