Commit Logs

A Few Thoughts On Efficient Code Review

Code review is a big part in daily software development here at Valley. Every production change requires at least a +2, ideally from someone who knows the project or the particular part of the codebase well, before it can be merged.

Code review serves multiple purposes. On a very high level, it helps to

  1. Enforce high coding standards.
  2. Find bugs at lower cost.
  3. Share knowledge between developers.

With all these benefits, code review comes at the cost of short-term development speed, although it saves time in a long-term perspective. To be frank, it will slow down your day-to-day work. This inevitably happens because 1) you need to wait for other people’s availablity and 2) you are not writing the perfect code (let’s agree to disagree on this one).

So the question becomes - how to make the code review process more efficient?

As a junior guy, I often find myself on one side of the table, where my code are reviewed by others, so I am eager to make this process more efficient. Over the course, I learnt a few things to make life a bit easier for both reviewers and myself. It would be a lie to say I can stick to these rules every time, but, when I did, they helped to move things faster.

Test it thoroughly

Review broken code is a waste of time for both you and the reviewers. Obvious bugs and compiler errors should already be handled prior to code review, if you had tested thorougly against your development/staging environment. This would save a significant amount of time and build trust between you and the reviewers. At the end of the day, the responsibility is on you to make sure no breaking changes/bugs are introduced. Code reviewers are there to help with their fresh pair of eyes for non-trivial buggy logics that might be hard to detect when look too closely.

Submit small patches

Breaking down changes into small patches not only speeds up the code review process, but also helps with the development process. On the developer side, it forces you to think critically about the structure of your change. Often times, by isolating small pieces of logic out of a large change, your code becomes cleaner and less error-prone with easy testibility. On the reviewers side, a small patch of code is easier to follow and helps to keep their eyes “fresh”. There is the human psychology part as well. People tends to procrastinate on big tasks and it is just much less intimidating to approve a 50 line of change than, say, a 1500 line one.

The goal of always submitting small patches is to ensure quick turnaround. With quality feedback within a day or two, this wouldn’t block the development of the next patches. Admittedly, things may not go smoothly every time. When the reviewers are delayed for whatever reasons, you may find yourself creating a large stack of small changes with the need of constantly rebasing on top of the previous patches. But, overall, this technique works well from my experience.

Include good commit message

Good commit message is the best way to communicate context about a change to fellow developers and reviewers. A commit message includes a subject line and a body. The latter is optional depending on the complexity of the change. The subject line summarizes the change and, if there is a body, it should focus on the what and why parts of the changes, as opposed to how (the code explains that). In this way, your reviewers can quickly get the context and general idea of the change so that they can spend most of their time on the actual code.

There are seven rules to write a good commit message (I copy&pasted from google). Note that these rules should be treated as code style guide - they should be enforced as part of the code review process. They help to maintain concise and readable commit logs, which is critical to any projects’ long-term success.

  1. Separate subject from body with a blank line
  2. Limit the subject line to 50 characters
  3. Capitalize the subject line
  4. Do not end the subject line with a period
  5. Use the imperative mood in the subject line
  6. Wrap the body at 72 characters
  7. Use the body to explain what and why vs. how

Summary

Like a lot of things, the key to make your reviewers life easier is to put yourself into their shoes. The things outlined here are the ones I would like to see if I were the reviewer. Now, imagine you were in that position, what kinds of code reviews make you happy? Sooner or later, you will be on the other side of the table. So, you might be better off starting to think about this question now and make the changes you would like to see happen.