Code reviews are awesome. They give team members insights into what changes are happening in the code base, reduce the number of bugs (and does so early on to reduce the cost of bugs), and allows team members to learn from eachother from reading other people’s code. The advantages are obvious, and I think all software teams should do them.
Of all the greatness though, one of the issues that has to be dealt with occasionally is a code review that you find a particularly large number of “issues” to address. Bugs being introduced, poor architecture decisions, UI/UX that is sub-par, maybe even something simple like inconsistent code style. Here are some thoughts on how to make those developer-to-developer discussions productive with no hurt feelings.
Automate the Dirty Work
Nobody wants to have to tell their co-worker to clean up syntax issues like trailing whitespaces or poor indentation. It can be difficult to tell if it’s just your personal preference or if it is a legitimate issue to address. In addition it’s a waste of time. If a developer is doing code review, they should be reviewing the meat of the code and design decisions and not syntax issues.
To automate our “style review” we have started using https://houndci.com built by thoughtbot. Hound is essentially a tool to take a set of defined rules (for example, “no trailing whitespaces”) and check that all new commits meet those rules. If they don’t, it adds a comment directly on the line of code in GitHub.
At the beginning there were a few “rules” that were overly strict and which we turned off to suit our development style, but now that we have it configured as we want it’s been great and helps clear the road for more effective code reviews.
It’s about the code, not the person
This is an obvious one, but important. When you’re doing code review it is just that — reviewing code. As developers we can become very tied to what we build. Pay extra attention to making sure no feelings are hurt.
Ask why
It’s important to remember that whoever’s code you are reviewing has probably put much more thought into the details of the code (and we all know how many details there truly ) than you have. So, respectfully ask them the why they wrote it that way and what their thought process was.
Recommend Self Reviews
A good practice is to have developers review their own branch before asking for code review from somebody else. A lot of times there are misc TODO’s that need to be cleaned up, commented out code to remove, debugging output to surpress, etc. A quick self review will catch all of those before somebody else has to spend their time doing that for them. In addition, the original developer will know the ins and outs better than anyone else and maybe be able to spot things others cannot.
Remember to Praise
Code review can be scary. Often, it is a series of critiques with very little positives. We should remember though that code review can be about more than just getting rid of the “bad things”, but also about keeping around the “good things”. If you find a particularly elegant piece of code don’t hesitate to praise it for what it is.