From YaddaWiki

Latest revision as of 07:21, 19 April 2021

The Code Review Process

To guarantee the highest quality of the developed software we use the code review process to accept every commit to a shared (master/development) branch.

As a tool for the process, we use github's pull requests or gitlab's merge requests, typically in so called shared repository model (See how to work with code repository in this kind of model).

The code review is part of the scrum process we use to manage our work. In order for the code review to be most efficient, neutral and objective, each developer is assigned a different colleague who will check their code when a new sprint begins.

Code Review has the highest priority. You should make a break in doing your own programming task to do a Pull Merge request that has been assigned to you. You can only postpone the CR if you have the consent of the person that requested the Pull Merge.

We use English to comment the code in CR (for both learning the language and being able to work in an international environment).

How you should review your peer's code

The easiest way (at least until you master the process) is to use the checklist below:

(Based on: http://java.dzone.com/articles/java-code-review-checklist)

General /clean code/

• Mind the Coding conventions
• Adhere to generally accepted naming conventions
• Use Intention-Revealing Names
• Classes should be small and focused (max. 20 methods)! (a getter and setter for a given field may be counted as one)
• Functions should be small and focused (max. 20 lines)!
• Mark code sections properly
• Avoid duplication of code
• Check parameters for validity
• Return empty arrays or collections, not nulls
• Minimize the accessibility of classes and members
• In public classes, use accessor methods, not public fields
• Use enums instead of int constants
• Always override hashCode when you override equals
• Use the new java.time date library whenever possible (instead the old java library)
• Group classes into packages by their functionality, not type of service
• Divide class methods into sections: getters, setters, private, logic etc.
• Use curly braces also for one-line code blocks*
• Minimize the scope of local variables
• Explain yourself in code - Comments (written in English)
• Make sure the code formatting is applied
• Refer to objects by their interfaces
• Avoid finalizers
• Use marker interfaces to define types
• Synchronize access to shared mutable data
• Prefer executors to tasks and threads
• Document thread safety

Exceptions

• Use Exceptions rather than Return codes or nulls
• Prefer runtime exceptions
• Use checked exceptions for (really) recoverable conditions and runtime exceptions for programming errors (and prefer the last)
• Favor the use of standard exceptions
• Don't ignore exceptions

Tests

• Write unit tests for every business method
• Write integration/functional tests for the prepared functionality
• Do not forget to adjust tests to changes in the existing functionalities
• Pay attention to duplicate test methods, make sure that the parametrized test methods are used where applicable

Security

• Beware injection inclusion - do not join parameters with query 'by hand', use dedicated mechanisms
• Beware cross site scripting attack - always escape untrusted input before printing (e.g. use <c:out in jsp)
• Validate inputs (for valid data, size, range, boundary conditions, etc)* Limit the accessibility of packages, classes, interfaces, methods, and fields
• Release resources (Streams, Connections, etc) in all cases
• Purge sensitive information from exceptions (exposing file path, internals of the system, configuration)
• Do not log highly sensitive information
• Limit the extensibility of classes and methods (by making it final)
• Input into a system should be checked for valid data size and range
• Avoid excessive logs for unusual behavior
• Consider purging highly sensitive data from memory after use
• Define wrappers around native methods (not declare a native method public)
• Make public static fields final (to avoid caller changing the value)
• Avoid exposing constructors of sensitive classes
• Avoid serialization for security-sensitive classes
• Guard sensitive data during serialization

Performance

• Limit database queries
• Count instead of list.size()
• Load only necessary objects (lazy loading) in the given situation
• Beware the performance of string concatenation
• Avoid excessive synchronization
• Keep Synchronized Sections Small
• Avoid creating unnecessary objects