How to Perform a Code Review

Code review is a careful, systematic study of source code by people who are not the original author of the code. It’s analogous to proofreading a term paper.

Code review really has two purposes:

• Improving the code. Finding bugs, anticipating possible bugs, checking the clarity of the code, and checking for consistency with the project’s style standards.
• Improving the programmer. Code review is an important way that programmers learn and teach each other, about new language features, changes in the design of the project or its coding standards, and new techniques. In open-source projects, particularly, much conversation happens in the context of code reviews.

Open-source projects like Apache and Mozilla widely practice code review, and it is also a common practice in the industry. At Google, you can’t push any code into the main repository until another engineer has signed off on it in a code review.

It is all about making code better. However, reviewing a peer’s code is easier said than done. Not to mention that running a review process can be a nightmare for team leads. For that reason, Offensive360 explains what to look for in a code review.

Create a Code Review Checklist

Utilize an active and structured code review checklist to ensure quality checks before approving code into the codebase. This checklist comprises predefined questions and rules that your team follows, providing a systematic approach to the review process. Your checklist may include:

• Readability: Are there any redundant comments in the code?
• Security: Does the code expose the system to a cyber attack?
• Test coverage: Is there a need to test more cases?
• Architecture: Does the code use encapsulation and modularization to achieve separation of concerns?
• Reusability: Does the code use reusable components, functions, and services?

Comments Where Needed

• A quick general word about commenting. Good software developers write comments in their code and do it judiciously. Good comments make the code easier to understand. Enhance its safety by documenting important assumptions, and preparing it for future changes.
• One kind of crucial comment is a specification, which appears above a method or above a class and documents the behavior of the method or class. In Java, this is conventionally written as a Javadoc comment, meaning that it starts with /** and includes @-syntax, like @param and @return for methods. Here’s an example of a spec:

/**
 * Compute the hailstone sequence.
 * See http://en.wikipedia.org/wiki/Collatz_conjecture#Statement_of_the_problem
 * @param n starting number of sequence; requires n > 0.
 * @return the hailstone sequence starting at n and ending with 1.
 * For example, hailstone(3)=[3,10,5,16,8,4,2,1].
 */
public static List<Integer> hailstoneSequence(int n) {
...
}

Fail Fast

Failing fast means that code should reveal its bugs as early as possible. The earlier a problem is observed (the closer to its cause), the easier it is to find and fix. As we saw in the first reading, static checking fails faster than dynamic checking and dynamic checking fails faster than producing a wrong answer that may corrupt subsequent computation.

The dayOfYear function doesn’t fail fast — if you pass the arguments in the wrong order, it will quietly return the wrong answer. In fact, the way dayOfYear is designed, it’s highly likely that a non-American will pass the arguments in the wrong order! It needs more checking — either static checking or dynamic checking.

Don’t Review more than 200-400 lines of code at a time

Reviewing more than 400 lines of code (LoC) can have an adverse impact on your ability to find bugs, and in fact, most are found in the first 200 lines. This limitation affected industry practices after Cisco identified it in a comprehensive study on code review.

The study found that, once developers reviewed more than 200 lines, their ability to identify defects waned (see the chart below).

Avoid Magic Numbers

There are really only two constants that computer scientists recognize as valid in and of themselves: 0, 1, and maybe 2. (Okay, three constants.)

We need to explain other constant numbers. One way to explain them is with a comment, but a far better way is to declare the number as a constant with a good, explanatory name.

dayofyear is full of magic numbers:

• The months 2, …, 12 would be far more readable as FEBRUARY, …, DECEMBER.
• The days of months 30, 31, and 28 would be more readable (and eliminate duplicate code) if they were in a data structure like an array, list, or map, e.g. MONTH_LENGTH[month].
• The mysterious numbers 59 and 90 are particularly pernicious examples of magic numbers. Not only are they uncommented and undocumented, they are actually the result of a computation done by hand by the programmer. Don’t hardcode constants that you’ve computed by hand. Java is better at arithmetic than you are. Explicit computations like 31 + 28 make the provenance of these mysterious numbers much clearer. MONTH_LENGTH[JANUARY] + MONTH_LENGTH[FEBRUARY] would be clearer still.

One Purpose for Each Variable

In the dayOfYear example, the parameter dayOfMonth is reused to compute a very different value — the return value of the function, which is not the day of the month.

Don’t reuse parameters, and don’t reuse variables. Variables are not a scarce resource in programming. Introduce them freely, give them good names, and just stop using them when you stop needing them. You will confuse your reader if a variable that used to mean one thing suddenly starts meaning something different a few lines down.

Not only is this an ease-of-understanding question, but it’s also a safety-from-bugs and ready-for-change question.

Method parameters, in particular, should generally be left unmodified. (This is important for being ready for change — in the future, some other part of the method may want to know what the original parameters of the method were, so you shouldn’t blow them away while you’re computing).

It’s a good idea to use final method parameters, and as many other variables as you can. The final keyword says that the variable should never be reassigned, and the Java compiler will check it statically.

For example:

public static int dayOfYear(final int month, final int dayOfMonth, final int year) { ... }

Use Good Names

Good method and variable names are long and self-descriptive. By making the code review itself more readable and using better names that describe the methods and variables, you can often avoid the need for comments entirely.

For example, you can rewrite

int tmp = 86400;  // tmp is the number of seconds in a day (don't do this!)

as:

int secondsPerDay = 86400;

In general, variable names like tmp, temp, and data are awful, symptoms of extreme programmer laziness. Every local variable is temporary, and every variable is data, so those names are generally meaningless. Better to use a longer, more descriptive name, so that your code reads clearly all by itself.

Follow the lexical naming conventions of the language. In Python, developers typically capitalize classes and use lowercase with underscores to separate words for variables. In Java:

methodsAreNamedWithCamelCaseLikeThis

variablesAreAlsoCamelCase

CONSTANTS_ARE_IN_ALL_CAPS_WITH_UNDERSCORES

ClassesAreCapitalized

packages.are.lowercase.and.separated.by.dots

Method names are usually verb phrases, like getDate or isUpperCase, while variable and class names are usually noun phrases. Choose short words, and be concise, but avoid abbreviations. For example, the message is clearer than msg, and the word is so much better than wd. Keep in mind that many of your teammates in class and in the real world will not be native English speakers, and abbreviations can be even harder for non-native speakers.

The leap method has bad names: the method name itself, and the local variable name. What would you call them instead?

Discover how Offensive360 can level up your code reviews:

Be sure not to see code reviews as just a means through which more experienced developers might provide advice to less experienced ones. Instead, it provides the opportunity for the whole team to grow, learn, and exchange information. In addition, if you use the one, we provide for the code review document, you can easily include this improvement in every single meeting, therefore ensuring that each code review is thorough.

In order to do better, you need to increase your teamwork. The Offensive360 team is able to eradicate knowledge silos and unintentional bottlenecks because to the fact that it tracks both the responsiveness of iterations and the amount of time spent on them. We'll show you how our expert can help organize your personnel around the most important activities, and automate the remainder of the work if you let us schedule a demo.