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

#1

Post by pinkunicorn » Thu, Mar 10, 2016, 7:54 AM UTC

I'm currently working at a startup that has a very bad code review culture. People either treat design/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.

How do I turn this around and build a healthy code review culture?

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

#2

Post by EliRivers » Thu, Mar 10, 2016, 10:01 AM UTC

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.

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

#3

Post by over » Thu, Mar 10, 2016, 10:05 AM UTC

EliRivers wrote: post #2

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 really like this, it's process engineering applied to software engineering. But how do you do a process review? :)

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

#4

Post by TerryADavis » Thu, Mar 10, 2016, 10:34 AM UTC

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

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

#5

Post by seren » Thu, Mar 10, 2016, 10:34 AM UTC

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.

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

#6

Post by grif-fin » Thu, Mar 10, 2016, 11:46 AM UTC

over wrote: post #3

EliRivers wrote: post #2

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 really like this, it's process engineering applied to software engineering. But how do you do a process review? :)

How easily did everyone agree to the coding standard purposed? I would be surprised and would have liked to see that coding standard if everyone agreed in the company with it.

I remember starting a project with close friends who knew each other well but we struggled on agreeing on some coding standard which everyone was happy.

People have different taste when it comes to formatting the code syntax and unless they really deep down agree to coding standard they may still like their previous habit they keep going back to them.

Overall I think it is tricky and I sometimes found it the argument of I like blue and you like red, what should we do now? Want to convince me to like red?

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

#7

Post by bzalasky » Thu, Mar 10, 2016, 12:01 PM UTC

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.

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

#8

Post by EliRivers » Thu, Mar 10, 2016, 12:12 PM UTC

grif-fin wrote: post #6

over wrote: post #3

Earlier quoted context omitted.

I really like this, it's process engineering applied to software engineering. But how do you do a process review? :)

How easily did everyone agree to the coding standard purposed? I would be surprised and would have liked to see that coding standard if everyone agreed in the company with it. I remember starting a project with close friends who knew each other well but we struggled on agreeing on some coding standard which everyone was happy. People have different taste when it comes to formatting the code syntax and unless they rea…

How easily did everyone agree to the coding standard purposed?

We did it by making it really, really small. No style guidelines. Two space, four spaces, braces in line, braces not in line? Don't care. It doesn't matter. If you're working on some code and you genuinely can't understand it because the layout is a mess, dump it through an auto-layout tool or the like. Honestly, so long as it's readable, arguing over two spaces for an indent or whether the if block first brace should be on the next line or not isn't worth the hassle.

We started with a handful of outright forbidden functions, and some rules on memory management. As time goes on, and various mistakes get made repeatedly, protections against them get added to the standard.

A small standard is better than no standard. A small standard is also better than a big standard.

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

#9

Post by pmiller2 » Thu, Mar 10, 2016, 4:06 PM UTC

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.

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

#10

Post by deeteecee » Thu, Mar 10, 2016, 6:34 PM UTC

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.