Simple Talk is now part of the Redgate Community hub - find out why

The Zen of Code Reviews: Best Practices

If you don't feel that you are getting helpful and comprehensive feedback from code reviews, it may well be your fault. Unless you are considerate to your reviewers in a number of ways, they might find it difficult to check your code and provide helpful advice. What ways? Michael Sorens outlines the eight golden rules that, if you follow them, might even even make your code a pleasure to review!

Introduction

In the first article that I wrote on code reviews ( The Zen of Code Reviews: Pre-Review Comments), I described how you could use “pre-review comments” to get more, and better, feedback from reviewers of your code, while making the reviewers’ job easier and less tiresome. This article has the same goals in mind but takes a broader perspective, covering a range of advice about what you should do before sending your code to your colleagues for review.

One: Include Only One Issue

No matter how much code there is to review-whether the issue you are working on requires modifying just one small function or modifying fifty or more files-give each issue a code review by itself. Even if you are working on several issues concurrently-and there are various, legitimate reasons to do that- it is much cleaner to put each issue into a separate code review once you are ready to have your code reviewed. Though it might seem as if you are getting some economy of scale by combining several small issues into one code review, that potential benefit is outweighed by the several advantages of having a single independent code review for each issue:

  • It encourages focus: you can use both the issue number and the issue title as the subject line of your code review. Your reviewer can then tell at a glance what the subject is and, with the issue number, can review any background information from your issue-tracking system.
  • It provides traceability: once your code has been reviewed and you are ready to commit to your source control system, you can then reference the single code review along with the very same issue number and title that you used in the code review.
  • It keeps things tidy: by committing to source control in the same vein, you then have the ability to back out a specific isolated issue or bug fix, should the need arise.
  • Finally, when you need to review your source code history (i.e. commit log) to find something, it is much easier to search, either programmatically or manually, when each commit has a single raison d’être.

Two: Include All of One Issue

This is the converse of the prior guideline. Once you establish the single issue that you are working to fix – or enhance or introduce – you should include everything in your code review, and your eventual code commit, that is connected to that issue. This would include such things as:

  • user interface changes
  • application code changes
  • documentation updates
  • help text revisions
  • database schema revisions
  • unit test additions
  • configuration file updates
  • build script changes
  • installer tweaks

Consider this list to be a starting point rather than an exhaustive enumeration! Add to it generously to meet your own team’s needs. Collect it all together into a single code review.

Everything needs to be included. You might think that it is only possible to miss such things as installer tweaks or config file updates: Surely you would never fail to include any changes to the code base proper, because omissions in the main code base would make your code fail to compile or your unit tests just fail. In reality, it is all too easy to miss items in some ancillary files, so you ought to get into the habit of checking. As an example, refactoring often entails having to rename things. Frequently when a colleague sends along a code review to me and claims to have refactored a widget provider into a ligature maker and a scimitar extruder, I will whip out my file scanner of choice and search the entire code base for “WidgetProvider” to see if there are any strays. I’ll include such things as supporting scripts or text files.By doing that, you are more likely to have included every relevant change, which again aids clarity when reviewing the commit history, investigating introduced defects, and so on. Most importantly, though, it supports this next principle:

Three: Really, Include All of One Issue

With source control, a commit should never break the build. If you use gated commits, where code must compile and unit tests must pass before a commit transaction in your source control system can complete, you are safe here. But not every source control system supports such gated transactions and even for those that do not every development group avails themselves of it. Sigh. However, with code reviews there is no equivalent of the gated transaction: you are free to omit any crucial files you please when shipping off a code review. This means that your reviewer might unwittingly be the target of GI thus making you unknowingly the target of GO (that’s GIGO if you put it together).

Consider the facilities of TFS’ source control (other source control systems are undoubtedly similar). Whilst preparing a code review in TFS, you have a list of pending file changes to work with. TFS lets you separate this list into two groups: included changes and excluded changes. Included changes, or what you might more easily think of as the active files list, are those files that TFS actions, notably commit or send code review, will affect. You are free to move files between the include and exclude lists, because not every pending change necessarily belongs to the issue you are working on at any given moment. Even if you are only working on a single issue, some of the files you touched should not be part of your code review. Perhaps, for example, you modified a generic configuration file so you could run your code on your local machine: That’s a type of change that would not be relevant to include in the code review you are preparing. With the necessary freedom to manipulate your active list, though, it is very easy to omit a relevant file inadvertently by leaving it dangling in the ‘excluded changes’ list.

Worse yet, TFS provides an even more extreme, ‘ very excluded changes list. Sometimes when you add a new file to your project in Visual Studio, it shows up just as you would expect in Solution Explorer, but TFS fails to add it automatically to the included changes list! If this is the case, TFS does not even deem it worthy of being in the excluded changes list. Rather, it puts it in a separate list of files that are in your tree but not managed by TFS. (This shows up as a “Detected: n add(s)” link just under the ” Excluded Changes” header, where n is the number of files it has detected.) You have to open that link, and promote the file(s) into TFS, which will make them appear in the included changes list.

Thus, you could easily send a code review with missing files and, if this error is not detected, then would proceed to commit with the same flaw and, indeed, break the build.

Four: Checkout Latest Code Before Preparing Your Review

Again let’s consider this next point in the more familiar context of source control: before you check in a set of changes you should check out the latest code from source control. Depending on how closely you work with colleagues, it is quite possible that while you were working on your issue, someone else touched some of the same files and committed those changes, and there may be conflicts that need resolving. (I’m talking about source code conflicts here; for interpersonal conflicts, you are on your own). Rather than find out about conflicts when you attempt to commit your changes, it is much better to do so pre-emptively.

In the code review arena, the situation is the same. When you are ready to prepare your code review, checkout the latest source, merge it in with your changes in your local workspace, and resolve any conflicts. This may necessitate some further changes in your code so it would be counterproductive to send out a code review before having done this crucial step.

Five: Review Your Review for Accuracy

Before sending a code review to your colleagues, do your own review first, file by file, line by line. I warm to Justin Ethier’s comments on the StackOverflow question What is the use of commit messages?, which is just as relevant for preparing a code review as it is for preparing a commit: “Even if I remember what I have changed, I will often do a quick ‘diff’ of a file before checking it in. I will briefly read it all over again to make sure that there are no ‘typos’, that I changed everything I meant to, etc. This is a simple way to review your code for those minor bugs that would otherwise find their way into your code.”

Indeed! Use the opportunity to check for things like:

  • Did your IDE reformat part of your source without you realizing it?
  • Did you need to tweak configuration files to test your code and forgot to reverse the tweaks?
  • Did your IDE Update the project files (e.g. *.csproj) on your behalf in ways that weren’t what you wanted?
  • Contrariwise, do you need to add some project file customizations that your team or company require to comply with your own build conventions?
  • If you use a tool that checks for code lint (e.g. Resharper), have you eliminated all its warnings?
  • Likewise; if you use a tool that checks spelling (e.g. Visual Studio Spell Checker or the ReSharper add-on ReSpeller), have you corrected all the errors or added your key words to its dictionary to quell the beast?
  • Did you leave any “TODO” comments (or whatever your keyword of choice is for work you mean to get back to before calling the work “done”)?
  • Have you left orphaned variables, properties, methods, or even files that are no longer needed?
  • Have you checked for any missing files?

Six: Apply Pre-Review Comments

I believe it is useful to distinguish between in-code comments, where you annotate existing code to explain obscure or tricky parts, and pre-review comments, where you explain non-obvious changes from the prior version to the current version. In practice, you could combine this step with step five. That is, as you walk through the code looking for all of the elements itemized in step five, also consider the nature and complexity of the code at the same time. Do this with an eye to your target audience-your reviewers. Specifically, answer the question: will your colleagues understand and appreciate the significance of this code change? Obviously, everyone has different levels of knowledge and experience, but you are likely to be familiar with your colleagues’ skill sets. So is the ramification of the code change clear enough to an informed reviewer? Most of the time the answer is likely to be “yes”, but sometimes you need to add some pre-review comments to guide your reviewers, to justify your choice, or to explain your great, new idea. The previous installment of this series was exclusively about these pre-review comments-see The Zen of Code Reviews: Pre-Review Comments for more.

Seven: Always Make Changes for a Reason

There must always be a reason for a change: Or, to put it another way, everything you do should have an issue or ticket or bug number in your workflow tracking system. For one, you will always have an issue number and issue title to comprise the subject line of your code review, as mentioned earlier. But perhaps more importantly, it helps you retain a focus on the business needs rather than just hacking a piece of code because you have a better idea. If you have a code idea that has business value-even if it is just addressing technical debt-get it on your workflow board! That forces you to write it down and be more concrete about what you plan to do. That, in turn, should help provide a notion of what “finished” means for the issue; all too often, such an undocumented or unplanned task can take “as long as it takes”. Having the issue in the system also lets other developers know why you changed something, because they can trace code changes back to your issue tracking system. Heck, it even lets you know what changed when you come back to it a month later, having forgotten all about it. Last, but not least, official process audits (ISO 9001 et al) typically require you to track all changes and document them. Coming full circle back to code reviews: when your work always has an issue number then your code reviews do, too.

Eight: Every Change Should Be Reviewed

If you and your teammates have seen the value of doing code reviews, then all changes should be subject to a code-review, no matter how insignificant. I recently had a code review come across my desk that consisted of a one-line change, a relatively short line at that-and it was not correct! This happened to be in a scripted language, and scripted languages tend to be more …um… forgiving in what they allow you to do. In this case, the developer missed a nuance of the particular operator he was using, so it actually changed the datatype of the result.

Summary

Code review is a valuable and important tool for developing high-quality code but, like many things in life, the value you get out of it depends on the effort you put into it. As I have discussed in this article, the task of preparing a code review should be a lot like preparing a source control commit. If you are diligent about your source control practices, you are probably well on the way to being diligent in your code review practices.

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