Types of Reviews

During the life cycle of a project, several types of reviews should be undertaken. While the majority of this chapter focuses on peer code reviews, you should be aware of the other types of reviews to ensure you’re not commenting too early (or too late) on various aspects of the project:

Design critique

Typically developers are not involved at this stage of the project; however, including a developer’s input may result in minor user interface enhancements that radically simplify the build.

Technical architecture review

A peer review of the underlying foundation for the code that is about to be built. At this stage, developers should be ensuring the data model is complete and can easily accommodate all parts of the build, and perhaps future features as well.

Automated self-check

Like spell-check, but for code; an automated self-check allows developers to ensure their code is following coding standards for the project. You may have additional testing suites that you want to run. The purpose of this type of review is to automate any type of review that could easily be caught by a machine check, instead of wasting time performing human checks.

Ticket-based peer code review

The majority of this chapter will be spent discussing this type of review.

Quality assurance/user acceptance testing

After the code review, the new feature will be merged into the development branch and make it available for testing by human testers. This user interface review is typically conducted on a special, nonproduction server.

Types of Reviewers

Depending on the size of your project, you probably have a variation on one of the following types of review processes (or maybe a combination of several):

Peer Review

We are all equals and equally able to review code and accept it to the project. We learn from one another and do our best work when we know our peers will be judging it later.

Automated Gatekeeper

Our code has test coverage. We trust our tests and only submit work we know will pass a comprehensive test suite. Typically we ask for a second opinion before the code is pushed into the test suite (for automated deployment).

Consensus Shepherd

Our community of coders is vigilant, and opinionated. We require consensus from interested parties before code can be marked as reviewed by the community. We may also have a testbot that is part of our community, making it easier for human coders to know when a suggested change meets minimum standards.

Benevolent Dictator

My code, my way. You are welcome to submit your suggestions, but I will review or have my lieutenants review your work with a fine-tooth comb. I enjoy finding your mistakes and rejecting your work. Only perfection is good enough.

Peer reviews should not be limited to those who are of equal stature on a team. The benefits will vary, but they can be extended to any combination of skill levels (Table 8-1).

Software for Code Reviews

The commands outlined in this chapter can be used with any Git hosting system. Detailed instructions for code hosting systems are outlined in Part III—including instructions on using GitHub (Chapter 10), Bitbucket (Chapter 11), and GitLab (Chapter 12). The code review capabilities of these systems are managed by pull requests or merge requests, and they are relatively lightweight, making them easy to use and integrate into most workflows.

If your reporting requirements are more explicit due to industry regulations, you may need to consider using a more formal code review and sign-off process. The following software packages focus explicitly on code review and sign-off. They are appropriate for the code review of extremely large software projects, and are likely more software than the typical project needs:

Gerrit

Used by Android, OpenStack, and Typo3, this review system is best for very large projects. There is a nice video presentation about its design (and limitations) by Dave Borowitz.

Review Board

Used by LinkedIn, the Apache Software Foundation, and Yelp, this software includes additional information about when lines of code were moved within the code base.

In addition to manual, peer review of code, it can also help developers to have automated tests to check their work against before requesting a peer review. Some open source projects, such as Drupal, have tools that can be used to verify that code conforms to coding standards (Coder). There are also for-pay services, such as PullReview for Ruby and bitHound for JavaScript, which are language specific but project agnostic.

Although we will be focusing on technical code reviews, increasingly non-technical reviewers are being included as part of the review process through customizable, on-demand build servers. A public example of this is the SimplyTest.Me service for Drupal. This platform allows people to deploy a test machine for 30 minutes at a time with a specific patch applied to the code so that they can review the changes proposed in the Drupal issue queue. These build servers can also benefit developers. Instead of conducting reviews sequentially, a reviewer can initiate the build process for a number of reviews all at once. Now the reviewer can avoid the (sometimes lengthy) procedure of building a local environment for each code review he or she is completing, by running the build process in parallel for all reviews that need to be completed. If this sounds appealing, you should read the Lullabot article on working with its pull request builder. Assuming your technology stack is a little different than theirs, a web search for “pull request builder” should get you pointed in the right direction.

Reviewing the Issue

Before beginning the local code review process, you should read through the description of the proposed changes in your team’s issue tracker to discover why the change was proposed. Is it a bug fix? How was the software broken? Is it adding a new feature? Who (and how) does the feature help? Understanding the problem before you look at the code will help you to answer “is this code the best way to solve this problem?” when the time comes.

Once you have a good understanding of what the code is supposed to be doing, it is time to set up your local environment so that you can replicate the “before” state. In other words, if it’s a bug, you should make sure you can replicate the bug in your testing environment. If it’s a new feature, you should make sure the feature doesn’t already exist (to be fair, it is pretty unlikely that two people will implement the exact same new feature).

The first step in reviewing someone else’s work is to verify how the code works currently. If you are testing a fix to a specific bug, that means you should start by replicating the bug. This is the only way you’ll know for sure that the new code fixes the problem, and it isn’t just a difference of environments making things appear to work. When you apply the new code, you also want to be able to catch any regressions, or problems, it might introduce. You can only do this if you know for sure that the problems were introduced in the code you just applied.

Once you’ve got your environment set up and you have confirmed the current state of the code, you can now check out a copy of the code you need to review.

Applying the Proposed Changes

In Chapter 2 you learned about several different access control models for Git. Your project might be setup such that the proposed review branch is in the main project repository (“Shared Repository Setup”), or it might be in a forked copy of the project repository (“Forked Repository Setup”). The instructions for the initial setup are different, so skip ahead to the section which is relevant to you.

$ git fetch

$ git fetch origin

$ git fetch origin 61524-broken-link:61524-broken-link

$ git clone https://example.com/<username>/<project>.git

$ cd project

$ git add remote donna https://example.com/donna/likesgin

$ get fetch donna

$ git branch --all

* master
  remotes/origin/master
  remotes/origin/HEAD -> origin/master
  remotes/origin/61524-broken-link

$ git checkout --track origin/61524-broken-link

Reviewing the Proposed Changes

First, let’s take a look at the commit history for this branch with the command log:

$ git log master..

This gives us the full log message of all the commits (starting with the most recent) that differ from the branch you’re comparing yours to. If there are not descriptive commit messages, return the work to the developer and ask her to add commit messages. There are instructions in Chapter 8 on how to write a great commit message, and instructions in Chapter 6 on how to reshape history (including adding new commit messages to previous commits with interactive rebasing).

To get a terse, but more complete history, examine only the current branch with the command log, but in graph form. By using the parameter --graph, you will get a sense of how this branch fits into the recent historical context of the project:

$ git log --oneline --graph

And finally, use the command diff. This command shows the difference between two points in your repository. These points can include commit objects, branch tips, and the staging area. We want to compare the current work to where you’ll merge the branch “to”—by convention, this is the master branch:

$ git diff master

When you run the command to output the difference, the information will be presented as a patch file. Patch files are ugly to read. You’re looking for lines beginning with + and -. These are lines that have been added or removed, respectively. You can scroll through the changes using the up and down arrows. When you have finished reviewing the patch, press q to quit. If you need an even briefer comparison of what’s happened in the patch, consider listing only the files, and then looking at the changed files one at a time:

$ git diff master --stat
$ git diff master filename

Let’s take a look at the format of a patch file:

diff --git a/jokes.txt b/jokes.txt
index a3aa100..a660181 100644
	--- a/jokes.txt
	+++ b/jokes.txt
@@ -4,5 +4,5 @@ an investigator.
 The Past, The Present and The Future walked into a bar.
 It was tense.

-What did one hat say to another's
-You stay here, I'll go on a head!
+What's the difference between a poorly dressed man on a tricycle and a
well dressed man on a bicycle?
+Attire.

The first five lines tell us we are looking at the difference between two files, with the line number of where the files begin to differ. There are a few lines of context provided leading up to the changes. These lines are indented by one space each. The changed lines of code are then displayed with a preceding - (line removed), or + (line added).

You can also get a slightly better visual summary of the same information we’ve looked at to date by starting a Git repository browser. I use gitk, which ships with the brew-installed version of Git (but not the version Apple provides). Any repository browser will suffice and many GUI clients are available on the Git website:

$ gitk

When you run the command, gitk, a graphical tool will launch from the command line. Click each commit to get more information about it. Many ticket systems will also allow you to look at the changes in a merge proposal side by side. Even if you love the command line as I do, I highly recommend getting an additional graphical tool to compare changes. For OS X, I like Kaleidoscope App because it also allows me to spot differences in images as well as code.

Now that you’ve had a good look at the code, jot down your answers to the following questions:

• Does the code comply with your project’s identified coding standards?

• Does the code limit itself to the scope identified in the ticket?

• Does the code follow industry best practices in the most efficient way possible?

• Has the code been implemented in the best possible way according to all of your internal bug-a-boos? It’s important to separate your preferences and stylistic differences from actual problems with the code.

With a sense of what the code changes are, you should go ahead and apply the changes to your local environment. In other words, display the rendered code however is appropriate for your project. Assuming it’s a website, now is the time to launch your browser and view the proposed change. How does it look? Does your solution match what the coder thinks he’s built? If it doesn’t look right, do you need to clear the cache, perhaps rebuild the Sass output to update the CSS for the project based on the changes you’re reviewing?

Now is the time to also test the code against whatever test suite you use:

• Does the code introduce any regressions?

• Is the new code as performant as the old code? Does it still fall within your project’s performance budget for download and page rendering times?

• Are the words all spelled correctly, and do they follow any brand-specific guidelines you have (e.g., sentence case versus title case for headings)?

Depending on the nature of the original problem for this particular code change, there may be other obvious questions you need to address as part of your code review. Ideally, your team will develop its own checklist of things to look for as part of a review.

Preparing Your Feedback

Do your best to create the most comprehensive list of everything you can find wrong (and right) with the code. It’s annoying to get dribbles of feedback from someone as part of the review process, so we’ll try to avoid “just one more thing” wherever we can.

Let’s assume you’ve now got a big juicy list of feedback. Maybe you have no feedback, but I doubt it. Release your inner critique and let’s get your review structured in a usable manner for your teammates. For all the notes you’ve assembled to date, separate them into the following categories:

The code is broken

It doesn’t compile, introduces a regression, it doesn’t pass the testing suite, or in some way actually fails demonstrably. These are problems that absolutely must be fixed.

The code does not follow best practices

You have some conventions, the web industry has some guidelines. These fixes are pretty important to make, but they may have some nuances the developer might not be aware of.

The code isn’t how you would have written it

You’re a developer with battle-tested opinions; but you can’t actually prove you’re right without getting out your rocking chair and launching into story time.

Submitting Your Evaluation

Based on this new categorization, you are ready to engage in passive-aggressive coding. If the problem is clearly a typo and falls into one of the first two categories, go ahead and fix it. You’ll increase the efficiency of the team by reducing the number of round trips the code needs to take between the developer and the reviewer. Obvious typos don’t really need to go back to the original author, do they? Sure, your teammates will be a little embarrassed, but they’ll appreciate you having saved them a bit of time. Hopefully the next time they won’t be so sloppy. However, if it’s the fourth or fifth time, do not fix the mistake. Your time is also valuable and your teammates need to check their own code before it gets to you.

If the change you are itching to make falls into the third category: stop right now. Do not touch the code. Instead, update the ticket where the problem was first identified and find out why your teammate took that particular approach. Asking “Why did you use this function here?” might lead to a really interesting conversation about the merits of the approach taken. It might also reveal limitations of the approach to the original developer. By starting a conversation, reviews can increase the institutional level of knowledge. By starting the conversation you’re also leaving yourself open to the possibility that, just maybe, your way of doing things isn’t the only viable solution.

If you “needed” to make any changes to the code they should be absolutely tiny and minor. You should not be making substantive edits in a peer review process. Make the tiny edits and then add the changes to your local repository as follows:

$ git add --all
$ git commit -m "Correcting <list problem> identified in peer review."

You can keep it brief because your changes should be minor. At this point, you should push the reviewed code back up to the server for the original developer to test and review. Assuming you’ve set up the branch as a tracking branch, it should just be a matter of running the command as follows:

$ git push

Update the issue queue as is appropriate for your review. Perhaps the code needs more work, or perhaps it was good as written and it is now time to close the issue queue.

Repeat the steps in this section until the proposed change is complete, and ready to be merged into the main branch.

Completing the Review

Up to this point, we’ve been comparing a ticket branch to the master branch in the repository. The final step in the review process will be to merge the ticket branch into the designated main branch (master) for the repository, and clean up the corresponding ticket branches.

Let’s start by updating our master branch to ensure we can publish our changes after the merge:

$ git checkout master
$ git pull --rebase=preserve origin master

Take a deep breath, and merge your ticket branch back into the main branch for your project’s repository. As written, this command will create a new commit in your repository history, which can be used to unmerge a public copy of the branch using the command revert if necessary:

$ git merge --no-ff 61524-broken-link

The merge will either fail, or it will succeed. If the merge fails, the original coders are often better equipped to figure out how to fix the merge errors, and you may need to ask them to resolve the conflicts for you. Tips on dealing with merge errors are covered in Chapter 6.

Once the branch is merged, you are ready to share the revised master branch by uploading it to the central repository:

$ git push

Once the new commits have been successfully integrated into the master branch, you can delete the old copies of the ticket branches both from your local repository and the central repository. It’s just basic housekeeping at this point:

$ git branch --delete 61524-broken-link
$ git push origin --delete 61524-broken-link

Summary

The peer review process can help your team. I have found it improves communication before ideas are committed to code. It fosters a mentoring attitude among team members. As a side benefit, it often encourages developers to start looking for ways to automate the process of testing to improve the efficiency of the reviews. Yes, it will take more time, but if you factor in the improvements I believe it’s time well spent.