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.

Tags: ,

  • 23903 views

  • Rate
    [Total: 24    Average: 4.4/5]
  • David V. Corbin

    A good start…
    The material you present would be an improvement over the practices I see in many organizations. At the same time I believe that are still many elements which I perceive as potentially problematic and inhibiting of the maximum effectiveness of code reviews.

    I would like to start by making a few declarative statements and seeing where we (and readers) align or diverge:

    1) "Never Break the Repository". 100% agree at least as far as "knowingly" – this means compiles and designated tests pass. Gated Checkin is a great tool for this, and IMPO should be required (provided one is using TFVC – other VCS will need other approaches to the same goal).

    2) "Commits should be associated to Tasks". 100% Agree (terminology is TFS specific, but concept is not).

    3) Tasks should nominally be between 3 and 6 hours of work. They may be shorter if there is something important to track related to the specific task. They may be longer if the work truly is a monolithic effort that can not be decomposed in a meaningful way.

    4) All commits should go through a Code Review. This is often debated, but I believe it to be true. The reviews can be synchronous (blocking) or asynchronous (non-blocking).

    5) Work should be subjected to peer review at multiple levels and from multiple viewpoints. This is a very important concept to keep the focus of a CODE review tight.

    6) The time to do an accurate code review increases exponentially with the size of the change as well as the size of the material impacted by the change.

    7) If the effort for a code review exceeds a threshold, it becomes increasingly likely that the review will be cursory rather than detailed and lose value

    8) The ability/effectiveness to act on feedback (from code review and other source) decreases rapidly with the passage of time for multiple reasons (worker has moved on, there are dependencies taken on the approach originally used, etc.)

    Once we evaluate these 8 items (and probably 1 or 2 I left out of the list), then it becomes possible to draw some conclusions…but not yet.

  • Joseph

    Some practical points.
    I agree with all of your points. The difficult thing can be to establish a culture of code review to begin with. A few practical items I’ve found that help.

    1. Establish a time (or two) each week during which code reviews will be held and tell all interested parties to block that time on their calendar. If you don’t use it, fine, but it ensures that everyone will be available, avoids the "couldn’t get time on everyone’s calendar" excuse for not doing one, and helps to get them done in a timely manner.

    2. Code reviews should be limited to about 1 1/2 hours in length. Much longer and people’s attention starts to wander. This would seem to conflict with items 2 and 3 above but if you can’t fit it in 1 1/2 hours, perhaps your review isn’t focused enough.

    3. Make sure people have reviewed the code beforehand and if it becomes apparent they haven’t, don’t be afraid to cancel the review and reschedule. Once you’ve done that a few times they’ll get the point.

  • David A. Gray

    Two Other Benefits of Single-Subject Code Reviews
    A code review that is confined to one topic is likely to require a shorter meeting, which makes it easier to get it into people’s calendars. Moreover, a single-subject review enables you to tailor your invitation list to people whose specialized knowledge or experience would be especially beneficial to you, and to the outcome of the review.

  • Kim C. Crosser (kim@thecrossers.net)

    Reviews at all levels build in quality
    I have run multiple software development organizations over the past 35+ years, from DoD integrated shipboard combat weapons and sensor systems to commercial point-of-sales systems to the national and state-wide jail/prison management systems.
    The one common factor I found that produced high quality code was implementing and enforcing reviews. In each of those organizations, I required the following:

    1. Design review: Before doing any actual coding, the developer had to present their proposed approach to either a peer or their supervisor (chief programmer). How does this design cover all the requirements for this module? What are the anticipated conditions the code would need to handle? How were errors or out-of-range conditions going to be detected and handled? What are the algorithms and data structures to be used? What are the complicated parts?

    2. Code review: Once coded and before being turned over for formal testing by the test team, the developer had to present the actual code to a peer/chief programmer. Walk the reviewer through the logic, show how the code implemented the requirements, show how the code handled exceptions, discuss any performance concerns, etc. When the implementation differed from the original design covered in the Design review, the developer also had to explain why the new approach was chosen.

    3. Test review: After the test team reported any failure, we held a review to determine the root cause. Was it a design flaw and should it have been caught in the Design review? Was it a coding flaw and should it have been caught in the Code review? Was this a simple typo? Why didn’t the developer catch this in unit testing?
    The test team’s job was to make sure that all the modules worked together in an integrated system – not to catch design/coding errors within an individual module.

    NOTE – when I say "present", I really mean "present". The developer was required to stand up and present their design/code to the other person(s) in a conference room, and all reviewers attending were required to note in writing that they reviewed and approved (or disapproved) the review, plus any comments/recommendations. These notes went into a development folder for the module. This ensured that the reviewers were engaged, as when a failure did occur, one of the questions we asked was "who reviewed this and how did this get past them?" Reviewers thus shared in the responsibility for the code quality with the primary developer.

    In every organization, the initial response to my enforcing these reviews was always "we don’t have time for these reviews". To which my response has always been – "but you do have time for rework later?"
    Once the reviews were in place and being performed regularly, everyone involved discovered that it did NOT take any more time than before and the volume of rework dropped dramatically. If you asked any of them after 4-6 months of following these processes, they would all agree that the reviews did not delay the development and were valuable.

    I know SEI isn’t a hot topic anymore, but it is interesting that SEI points out that up through SEI Level 2, additional software engineering practices add overall project time. However, when reviews are introduced as part of SEI Level 3, they note that process time then drops below the original baseline. When my team was acquired by a much larger company, their QA team rated us as an SEI Level 3 organization.

    FYI – I also tracked "returns" (number of times the same code module was failed by the test team). While developing a national prison/corrections management system for a foreign government, I fired one developer because he consistently averaged over three times the number of "returns" as any of the other developers.

    As a side benefit – entirely intended – the reviews disseminated and encouraged good design and coding practices, and disseminated knowledge about different modules across multiple persons.

    I always loved the quote – "Trying to improve quality by adding more tests is like trying to lose weight by standing on the scale more often." Quality needs to be built in from the ground up and reviews are key to that.