Notes on Code Reviews

Russell Bateman
February 2014
last update:

Best practices

Noted from an article by Vlad Mihalcea.

  1. Code review isn't testing: Code review is a developer-to-developer business and it doesn't involve any testing. Code review should verify that task requirements are met in the cleanest possible way.
  2. Don't tell what to code review: Don't tell a tester what to test, don't tell your peer what to review. The magic of peer review comes from your peers own perspective on the current task design and implementation. Two minds are always better than one.
  3. Always check all changes: Bugs may be hidden anywhere. Search for them diligently. Get the whole picture by going through all changes.
  4. Requirements first: Requirements are the most important driving force. It's what the customer is paying for. If the current changes are suboptimal you need to reopen the issue. If you happen to spot other code sections that need to be refactored you should create new issues instead of reopening the current one. The "single responsibility principle" applies to tasks as well as to coding.
  5. One-to-many activity: If you can't make sure you grasp the intent of the changes it's safer to ask somebody else to review it further.
  6. A way of learning: Code review is a great learning technique especially on large projects. Ideally you should become familiar with every aspect of your project, but if the project is too large you can at least specialize in multiple modules.

Code-review checklist

Items and categories from these references:

The elements in this check-list are categorized.

Clean code...

Use intention-revealing names Meaningful Names
Pick one word per concept Meaningful Names
Use Solution/Problem Domain Names Meaningful Names
Classes should be small Classes
Methods should be small Functions
Do one thing Functions
Don't repeat yourself (avoid duplication) Functions
Explain yourself in code Comments
Make sure the code formatting is applied Formatting
Use exceptions rather than return codes Exceptions
Don't return null Exceptions

Security...

Make class final if not being used for inheritance Fundamentals
Avoid duplication of code Fundamentals
Restrict privileges: Application to run with the least privilege mode required for functioning Fundamentals
Minimize the accessibility of classes and members Fundamentals
Document security related information Fundamentals
Input into a system should be checked for valid data size and range Denial of Service
Avoid excessive logs for unusual behavior Denial of Service
Release resources (Streams, Connections, etc) in all cases Denial of Service
Purge sensitive information from exceptions (exposing file path, internals of the system, configuration) Confidential Information
Do not log highly sensitive information Confidential Information
Consider purging highly sensitive from memory after use Confidential Information
Avoid dynamic SQL, use prepared statement Injection Inclusion
Limit the accessibility of packages,classes, interfaces, methods, and fields Accessibility Extensibility
Limit the extensibility of classes and methods (by making it final) Accessibility Extensibility
Validate inputs (for valid data, size, range, boundary conditions, etc) Input Validation
Validate output from untrusted objects as input Input Validation
Define wrappers around native methods (not declare a native method public) Input Validation
Treat output from untrusted object as input Mutability
Make public static fields final (to avoid caller changing the value) Mutability
Avoid exposing constructors of sensitive classes Object Construction
Avoid serialization for security-sensitive classes Serialization Deserialization
Guard sensitive data during serialization Serialization Deserialization
Be careful caching results of potentially privileged operations Serialization Deserialization
Only use JNI when necessary Access Control

Performance...

Avoid excessive synchronization Concurrency
Keep Synchronized Sections Small Concurrency
Beware the performance of string concatenation General Programming
Avoid creating unnecessary objects Creating and Destroying Objects

General...

Use checked exceptions for recoverable conditions and runtime exceptions for programming errors Exceptions
Favor the use of standard exceptions Exceptions
Don't ignore exceptions Exceptions
Check parameters for validity Methods
Return empty arrays or collections, not nulls Methods
Minimize the accessibility of classes and members Classes and Interfaces
In public classes, use accessor methods, not public fields Classes and Interfaces
Minimize the scope of local variables General Programming
Refer to objects by their interfaces General Programming
Adhere to generally accepted naming conventions General Programming
Avoid finalizers Creating and Destroying Objects
Always override hashCode when you override equals General Programming
Always override toString General Programming
Use enums instead of int constants Enums and Annotations
Use marker interfaces to define types Enums and Annotations
Synchronize access to shared mutable data Concurrency
Prefer executors to tasks and threads Concurrency
Document thread safety Concurrency
Valid JUnit/JBehave test cases exist Testing