Last week, the Google Testing Blog published an excellent article on some good practices when sending out a code review. I want to share a little bit about the things I learned from the view point of a code reviewer.
Set a bar
Like many other tasks, having a clear goal or target gives every one a better sense of what progress looks like. Of course the goal of code review is to have cleaner code, more readable code, and more efficient code. But how clean is clean? How readable is readable? Surely no code is perfect, and certainly no code can please everyone.
My personal bar for approving a code review: When I’m on call and get woken up at 2am a few months from now, will I be able to reason about and navigate through these changes? And will this change increase or decrease the chances of me being woken up at 2am?
Having a consistent bar like this allows me to get more efficient with reviewing code, instead of arbitrarily poking holes at a changeset.
Why is this changeset necessary?
This is always the first question that I seek to answer. To me as a reviewer, this is so important for getting a good sense of the whole picture, and allowing me to evaluate the alternative solutions against the changeset. I love it when the code author writes a good description about the problem the changeset is trying to solve; and if it’s a large change, a brief summary of the approach.
Too often, developers send out changesets describing what is being changed, and fail to mention why the changesets are necessary at all. Sometimes, the author would say “see bug #3423” when asked for more information. Yes, I have look through the bug you linked. And no, I don’t have time to read through the 20 comment exchange you had while investigating the problem.
Coding style
Unless the coding style is really obscure and far from the norm, I generally leave it alone. Again, this comes back to how comfortable I am with the readability of the changeset if woken up at 2am. This is one thing that has made reviewing code so much more pleasant. The code author doesn’t feel like you’re imposing your personal preferences on him or her, and less ego gets involved in the process.
I used to leave comments like “nit: space here”. I don’t anymore. If the team really cares about coding style, setup a good linter.
Comment tactfully
-
Use “we” instead of “you”. This is a team effort after all. I try not to make the comment sound like I’m pointing fingers and playing the blame game. For example: “Can
youwe consider doing XYZ instead?” or “Why don’tyouwe put these values into a constant?” Try reading the two versions out loud. -
Ask clarifying questions before suggesting a different approach. For example: “Maybe I’m missing something here, but don’t we already have module ABC doing the same thing?”
-
Cite existing pattern. For example: “Let’s follow the existing convention to have these values as constants up top.”
Of course these things also depend on the relationship between the code author and the reviewer. I’m much more of a dick when reviewing code written by someone I’m close with :).
Stop typing, and discuss in person
As a general rule of thumb, if a line of code has more than 3 or 5 comments, I would discuss with the author in person, or over IM (still typing, I know). Otherwise, public code reviews (where you whole team can see) can quickly turn into a pissing contest. By that point, nobody wants to lose face, and thus ego will involve and things turn from bad to worse.
After reaching a consensus, I would go back to the code review comment and say something a long the lines of “Discussed offline. We agreed that…”