Guidelines for Code Reviewers

The overall aim of the code review process is to increase the quality of the codebase.

Not 'does it work?' but 'is it easy to understand?' and 'is it simple?'

It is not the responsibility of the code reviewer to ensure that there are no bugs before code is committed, nor even that the code functions at all. These are 100% the responsibility of the person submitting the code. If the reviewer wants to undertake these checks then that is great, but beyond what we expect of code reviewing.

The code reviewer should primarily assess the maintainability of the code submitted, which means asking the following basic questions:

• is it easy to understand? (could it be made easier to understand?)
• does it follow best practices? (below)
• is it well-documented?
• does it follow code conventions?

The enemy of both code maintenance and code robustness is complexity. Your task as code reviewer is to find complexity and complain about it.

The code reviewer can also assess by inspection other important aspects of the code, such as:

• modularity
• resource efficiency

Caveat: as a code reviewer prioritize comprehensibility over efficiency. In other words, as is often said, we should aim to 'make it right, then make it fast' (not to try it the other way around).

Best Practices

There is a huge amount of advice regarding best practices, for example at: javapractices.com. We can't expect that submitted code is checked against all best practices. Here, for emphasis, are what we regard as the most important issues:

1. Length of code

Probably the most important issues to look at relate to code length:

• class size - look at the largest class: could it usefully be split into several smaller classes?
• method length - look out for method definitions consisting of many lines of code. It is likely that refactoring of these into smaller methods could improve the ability of someone new understanding what the code is doing.

2. Code organization

Another issue can be package structure. Classes should be organized into a package structure which is comprehensible. There shouldn't be too many packages, nor too few. As a rough guide, if a package contains less than 4 or more than 8 classes, it may be worth rethinking.

3. Naming

Another very important issue which is often under-rated is naming. Are packages, classes, methods and variables in your view, optimally named? That is,

• do you understand all the names?
• do the class and method names express their purposes well?

As a code reviewer, you might fear that you are being "picky" by complaining about names, but naming is a critical factor in the maintainability of code. If names are well-chosen, they serve as abbreviations of the functionality they name, which assists enormously in comprehending the whole. If a method is not well-named, one has to read each line of code to understand what it does, which is a very inefficient alternative.

Checklist

• Is the person submitting the code on the list of people who have confirmed transferral of copyright of code submitted?
• Am I having difficulty in understanding this code?
• Is there any complexity in the code which could be reduced by refactoring?
• Is the code well organized in a package structure which makes sense?
• Are the class names intuitive and is it obvious what they do?
• Are there any classes which are notably large?
• Are there any particularly long methods?
• Do all the method names seem clear and intuive?
• Is the code well-documented?
• Does it follow common Java code conventions?
• Does the code submitted introduce unnecessary dependencies which reduces the modularity of the codebase?
• Are there ways in which this code could be made more efficient?
• Are exceptions simply swallowed anywhere, rather than being handled explicity?