The Zen of Code Reviews: Pre-Review Comments

Code Reviews can have a great deal of benefit if they are done well and thoroughly. They are done best if it it isn't a chore for the reviewers. If you make as easy as possible for them by explaining the background to the edits and pointing out the significant changes, then the process goes far better for all: But how would you go about doing that? Michael Sorens explains.

Introduction

It is challenging to create good software, but there are many tools that you use every day to help you do the job. The editor helps you get the syntax right; the compiler helps you get the references right; unit tests help you get the logic right, and so forth. The word “right” implies that it is an absolute state, whereas it is really a matter of degree. You can build a piece of barely adequate code, or you can build a robust, extensible, performant piece of code. One very effective way to improve your code quality is to have your code reviewed by your teammates using a code review tool, which can help you to integrate the task into your development process. However, most people do not use a code review tool the right way, because the wrong way seems more “natural”.

This article explains how to use a code review tool to get more and better feedback from reviewers, while making their job easier and less tiresome. The techniques that I’ll describe are simple, easy to learn and easy to apply: They are just not very obvious-until you know them. Once you can apply them, you can get reviewers of your code to do more with less effort! I’ll be using TFS as an example, but the general principles that I describe will apply to whatever code review tool you use, or computer language you’re coding in.

The Right Comment for the Right Job

2158-general_comment-a9cd15d6-1afe-4f87-With the TFS code review tool, you select the set of files that you would like to have reviewed: You select your reviewers, you specify a subject, and you then add a single, global description that provides any notes you wish to convey to your reviewers. In this code-review description, you can type as much as you want, and it is certainly useful to give a high-level context to your changes. You are limited in what you could, or should, put in a general, global comment attached to your code review. It should be broad in scope, but brief and to the point; setting the tone for everything that is to follow.

However, that code review description is not sufficient for an effective review: you need to be able to point out specifics as well: ‘But that is easy’, you counter. ‘Just put some comments in the code for anything that might be tricky or obscure’. These in-code comments are, again, certainly very useful, but still insufficient. In-code comments are excellent for explaining what is but the final type of comment-what I call pre-review comments-explain what changed, which is arguably the most critical aspect of a code review.

Type

Details

When to Use

In-code Comments

Annotate existing code to explain obscure or tricky code.

Before creating your code review

Code-Review Description

Single, general comment attached to a code review when you create it.

When creating a code review, specified along with the reviewers

Pre-Review Comments

Explain non-obvious changes from the prior version to the current version.

After creating a code review but before sending to reviewers

You already know how to add two of these types, in-code comments and a code-review description. It is less obvious how to add the third type, pre-review comments, in TFS: It doesn’t seem natural, which is why it doesn’t occur to the developer to use them. There is no button or field or control for it; it is simply a process, and a simple one at that:

  1. Assign yourself as the sole reviewer.
  2. Send the review-to yourself.
  3. Open the review, review each change, and add comments explaining what you did.
  4. Send your completed comments-to yourself.
  5. Add other reviewers.

That is, the first thing you do is swap your developer hat for your reviewer hat. However, unlike your intended reviewers, who will examine all your changes and raise questions, your job is to examine all your questions and supply answers: in other words, annotate the code to explain why significant changes were made. The process of sending your completed comments to yourself (step D) in effect publishes or embeds your comments in the code review. When you then add additional reviewers (step E) they are receiving the code changes bundled together with your comments.

Pre-review comments are just as valuable to a code reviewer as in-code comments are to a code reader (the former being interested in code changes and the latter in code content). A code reviewer is a superset of a code reader, so one would gain insight from both in-code comments and pre-review comments. However, they are quite distinct. In-code comments describe what is present, whereas pre-review comments describe why it changed.

Why is this important? A reviewer’s job is to be able to provide useful feedback, which means that the reviewer must understand the issue that your code review addresses, then peruse your changes (whether really relevant to the issue at hand or not), and finally figure out what your changes mean in context, i.e. the ramifications of those changes. As the author, you have an unfair advantage. You already know the issue that your code addresses. You know which changes are significant and which are just noise. You know most (or sometimes all) the ramifications of your changes: So why not share that valuable insight with your code reviewers? Share everything-using pre-review comments-so your reviewers no longer need to first learn by discovery how to get to the point where you started, and then on from where you started to where you ended.

Example #1

Imagine that you are working on a web project and make this CSS change:

Original

margin: 8px

Revised

margin: 15px 8px 8px 0

What do you see, right in front of you? The answer to that is not quite as obvious as it may seem. I suggest that the answer is “It depends.” Are you a web developer? If not, perhaps you have still come across a little bit to CSS nonetheless; or perhaps you do not even know what the acronym CSS stands for. Even if you are a web developer, maybe you work with CSS and maybe you do not. If you know CSS, how experienced are you? Do you know the difference between the one-parameter margin property and the four-parameter margin property? Even if you know what the four-parameter margin property is used for, do you know the order of the parameters?

So what you see above could be anything from “a line of text” to “a change in the top and left margins for a particular CSS class”. Let us put that into a more targeted context. Let’s assume that you make the above change and send that out for code review. Typically, the act of sending out a code review involves, naturally enough, sending out your code changes-and nothing else. That is, the reviewer would see just this:

Original

margin: 8px

Revised

margin: 15px 8px 8px 0

But consider:

  • Even though your reviewers are developers, they might not know what this “margin” means.
  • Even if web developers, if they have not seen the page they would not have a clue which margin you are changing.
  • Even if familiar with the web page and which margin, they might not have an appreciation as to why. Why those numbers? Why does that change fix the issue? For that matter, what is the issue?

To make things easier for the reviewer, annotate the change with a comment providing both context and rationale for the change. For example:

Original

margin: 8px

Revised

margin: 15px 8px 8px 0

“Previously there was an 8-pixel border uniformly around the search form. Revised to increase the top border to 15 for better visual separation from the text above, and to eliminate the left margin completely so it now aligns with the other elements on the page.”

Thus, without needing to be familiar with the page, and without even being a web developer, a reviewer can provide useful feedback on this change!

Now, you could argue that that you could make the very same comment as an in-code comment, perhaps phrased like this (by just removing any temporal and comparative references from the above version):

“Set the top border to 15 for visual separation from the text above, and eliminate the left margin completely so it aligns with the other elements on the page.”

In this example you do get essentially the same impact; the next example shows that that is not always the case.

Example #2

Say you are working on a PowerShell project and make this bug fix.

Original

Set-Version $environment $package

If ($result.Success) { …

Revised

$result = Set-Version $environment $package

If ($result.Success) { …

Your reviewers could probably guess what happened there-but they should not need to! As the author of the change, you already know exactly what changed and why-so just state it. Here is one possible version of a pre-review comment:

“Bug fix: The cmdlet output was not being captured so it went to the pipeline; that’s why the output stream showed a blank line as reported in the defect report. But I found even more insidiously (and not in the defect report!), the next line was then acting on the success/failure of the prior operation, not the current one!”

In contrast to the prior example, this does not lend itself to an equivalent in-code comment. In fact, none of this statement is appropriate to embed in the code itself; it only makes sense in the context of the code review, as a pre-review comment.

Pre-review Comments Increase the Signal-to-Noise Ratio for Reviewers

Besides providing vital details on a particular line of code or component or architectural issue, pre-review comments address another vital aspect of a code review that can greatly enhance your reviewers’ experience: the signal-to-noise ratio.

A signal-to-noise ratio compares the level of a desired signal to the level of background noise (Wikipedia). In the context of a code review, the signal is the set of changes that meet the purpose of the code review. The noise is the set of incidental changes that you made for a variety of other legitimate reasons: perhaps because you were already touching the file, or because you thought that such cosmetic adjustments would make your real changes easier to follow. Alternatively, it might just have made the code a bit tidier, or could have happened because your text editor reformatted the code for you automatically. Such changes might include: adjusting an indent, adding or removing blank lines, changing a method name to be more restrictive, changing the case of a variable name, or a variety of other refactoring changes (such as introducing or removing a variable from a method, eliminating magic strings/numbers, etc.).

Consider, for example, this single file opened from a TFS code review:

2158-diff1-a078fc9b-eeaa-4978-adf4-e14de

This one file shows quite a lot of changes (per the difference bars at right)… but it is almost pure noise in this case!

The changes in the top half are just auto-formatting (whitespace) done automatically by the text editor. The changes in the bottom half are just a few functions that were relocated to a new class.

2158-diff2-00f19050-46ca-4431-8c1b-be984

Did you notice what I just snuck in: those two sentences just explained exactly what to convey to your reviewers. That reduces the review time of this file to the time it takes to read those two sentences.

Now let’s take a step back and look at a complete list of files in a sample code review from the TFS Team Explorer panel, shown at right. This code review includes many files with changes. Many of them here, perhaps 75%, are present only because one class was relocated or renamed, so the only change is really just a using statement at the top of the file.

But think about your audience. After going through ten or so of such files, do you really think they are going to diligently keep going through every single one? Not only do you gain from a more thorough review, but also it is your job to help them by reducing the noise. One possible solution: for the (say) six files that are significant add a comment “Relevant changes in this file”. Implicitly you are saying that only those files so marked need to be reviewed carefully, but all the other ones that were touched are also included should you wish to scan them briefly as well.

Because the comments that are added in TFS show up directly in the Team Explorer panel, it is quick and easy to select and view just those important changes.

Value in Reviewing Your Own Code

To produce pre-review comments, you need to go over your code and meticulously review the changes line by line… in other words, do just what you want your reviewers to be doing! Surely, though, this is something you should do anyway? Although the objective of a code review, of course, is to have other sets of eyes review your work, you will be surprised at how effective it can be to switch roles and exchange your developer hat for a reviewer hat. Here are just a couple examples.

  • As a reviewer, you will likely take the list of files as presented by TFS and walk through them, one by one, change by change. It is rather unlikely that you wrote the code in the same order so this already forces you to have a different perspective.
  • When was the last time you looked at changes you precipitated in the .sln or .csproj files as a developer? Much more likely that you have done so as a reviewer. ( Well, perhaps 🙂 It is not uncommon that Visual Studio makes changes in those files that are unnecessary for your particular project and they do, in fact, need your review. Alternately, perhaps when creating a new project, you need to make some manual customizations to a .csproj file per your own company conventions and a review is important to make sure they were not overlooked.
  • Did you ever leave a piece of code not-quite-finished, meaning to return to it before sending off your work for review but forgetting to do so? Doing your own code review while preparing your pre-review comments almost guarantees you will catch these types of errors.

Generic and Targeted Comments

To get you started, here are some generic samples from my list of pre-review comments:

  • From prior code review
  • Ignore this change
  • Added to quiet ReSharper
  • Not relevant to this code review
  • Relevant changes in this file
  • This is the crux of the issue
  • Region of deleted text was moved to…
  • Description is taken from the requirements for the issue.

…and these examples are more targeted …

  • Moved the tooltip to encompass the whole header rather than just the title phrase
  • Fixed discrepancy between the test name and what is being tested
  • Not terribly satisfied with all the method names in this class; if you have ideas let me know!
  • Not sure how much of a code smell this emits but it gets the job done.
  • Only substantive change for this code review is adding .trim() to these 2 lines.

Conclusion

A good team-based developer makes code reviews as easy as possible for the reviewers. With a minor effort applied to your code reviews, you can go quite a bit towards getting more and better feedback for your code reviews. For example, just point out the few key files or the few key lines that a reviewer should focus on. Take it to the next level-explain the architectural choices you made-and you can gain additional valuable feedback at the strategic level. Like many things in life, you get out of it what you put into it. I believe-and articles abound supporting this-that code reviews are valuable, so it is well worth the investment of time.

How you log in to Simple Talk has changed

We now use Redgate ID (RGID). If you already have an RGID, we’ll try to match it to your account. If not, we’ll create one for you and connect it.

This won’t sign you up to anything or add you to any mailing lists. You can see our full privacy policy here.

Continue

Simple Talk now uses Redgate ID

If you already have a Redgate ID (RGID), sign in using your existing RGID credentials. If not, you can create one on the next screen.

This won’t sign you up to anything or add you to any mailing lists. You can see our full privacy policy here.

Continue