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

24 points by pinkunicorn 10 years ago | 16 comments

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?

EliRivers 10 years ago |

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 10 years ago | |

"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 10 years ago | | |

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

over 10 years ago | |

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

grif-fin 10 years ago | | |

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?

bzalasky 10 years ago |

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 10 years ago |

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.

pmiller2 10 years ago |

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 10 years ago |

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 10 years ago |

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 10 years ago |

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 10 years ago |

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 10 years ago |

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 10 years ago |

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