Public:Code Review
From YaddaWiki
Contents |
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.
We use English to comment the code in CR.
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 the code when a new sprint begins.
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)!
- 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
- 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
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 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
