Public:Code Review

From YaddaWiki

(Difference between revisions)
Jump to: navigation, search
(→Tests)
 
(24 intermediate revisions not shown)
Line 1: Line 1:
-
= The Code Review Process in GitHub =
 
-
Pull Requests in GitHub's Shared Repository Model.
+
= The Code Review Process =
-
A nice description of the method used: http://wujingyue.blogspot.com/2012/12/pull-request-and-code-review-in-githubs.html.
+
-
In short the process of code review is as following:
+
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.
-
# Create a feature branch: **_git checkout -b feature_branch_name_** (feature_branch_name should start with your github login)
+
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 '''[[Public:Working with code repo|how to work with code repository]]''' in this kind of model).
-
# Make your changes
+
 
-
# Commit your changes
+
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.
-
# [Optionally, so the pull request will be more up-to-date] Clean up your history: **_git rebase master_**
+
 
-
# Run tests:  _**gradlew clean test**_ and _**gradlew clean slowTest**_
+
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.
-
# Push the changes: _**git push origin feature_branch_name**_
+
 
-
# Issue a pull request on github (master as the base branch, feature as the head branch)
+
We use English to comment the code in CR (for both learning the language and being able to work in an international environment).
-
# Wait for changes to be accepted. If you are asked to revise your changes, edit your branch and push the changes.
+
 
-
# Clean up your history: **_git rebase master_**
+
= How you should review your peer's code =
-
# Merge the feature branch into master: _**git checkout master**_ and then _**git merge feature_branch_name**_ (or **git merge --squash feature_branch_name**)
+
 
-
# If merge was done with **--squash** option, then commit merged changes and modify commit message: _**git commit**_
+
The easiest way (at least until you master the process) is to use the checklist below:
-
# Pull the changes from origin: _**git pull --rebase origin**_ (or git fetch origin and git rebase origin/master)
+
 
-
# Run tests before push:  _**gradlew clean test**_ and _**gradlew clean slowTest**_
+
 
-
# Push the changes to the master branch: _**git push origin master**_
+
(Based on: http://java.dzone.com/articles/java-code-review-checklist)
-
# Delete the feature branch: _**git branch -D feature_branch_name**_ and then _**git push origin :feature_branch_name**_
+
-
# [Optionally] Delete references to remote branches that do not exist in the repository anymore: _git remote prune origin_
+
-
= The Code Review Common Checklist =
 
-
Based on: http://java.dzone.com/articles/java-code-review-checklist
 
-
 
== General /clean code/ ==
== General /clean code/ ==
-
* '''Mind the project coding conventions'''
+
* '''Mind the '''[[Public:Coding conventions|Coding conventions]]'''
* '''Adhere to generally accepted naming conventions'''
* '''Adhere to generally accepted naming conventions'''
* '''Use Intention-Revealing Names'''
* '''Use Intention-Revealing Names'''
-
* '''Classes should be small and focused (max. 20 methods)!'''
+
* '''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)!'''
* '''Functions should be small and focused (max. 20 lines)!'''
 +
* '''[[Public:Class_code_sections|Mark code sections properly]]'''
* '''Avoid duplication of code'''
* '''Avoid duplication of code'''
* '''Check parameters for validity'''
* '''Check parameters for validity'''
Line 41: Line 35:
* '''Use enums instead of int constants'''
* '''Use enums instead of int constants'''
* '''Always override hashCode when you override equals'''
* '''Always override hashCode when you override equals'''
-
* '''Use the org.joda date library instead of standard java date library'''
+
* '''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'''
* '''Group classes into packages by their functionality, not type of service'''
* Divide class methods into sections: getters, setters, private, logic etc.
* Divide class methods into sections: getters, setters, private, logic etc.
* Use curly braces also for one-line code blocks*
* Use curly braces also for one-line code blocks*
* Minimize the scope of local variables
* Minimize the scope of local variables
-
* Explain yourself in code - Comments
+
* Explain yourself in code - Comments (written in English)
* Make sure the code formatting is applied
* Make sure the code formatting is applied
* Refer to objects by their interfaces
* Refer to objects by their interfaces
Line 54: Line 48:
* Prefer executors to tasks and threads
* Prefer executors to tasks and threads
* Document thread safety
* Document thread safety
-
 
== Exceptions ==
== Exceptions ==
Line 60: Line 53:
* '''Use Exceptions rather than Return codes or nulls'''
* '''Use Exceptions rather than Return codes or nulls'''
* '''Prefer runtime exceptions'''  
* '''Prefer runtime exceptions'''  
-
* '''Use checked exceptions for (really) recoverable conditions and runtime exceptions for programming errors'''
+
* '''Use checked exceptions for (really) recoverable conditions and runtime exceptions for programming errors (and prefer the last)'''
* '''Favor the use of standard exceptions'''
* '''Favor the use of standard exceptions'''
* '''Don't ignore exceptions'''
* '''Don't ignore exceptions'''
 +
== Tests ==
== Tests ==
Line 69: Line 63:
* '''Write integration/functional tests for the prepared functionality'''
* '''Write integration/functional tests for the prepared functionality'''
* '''Do not forget to adjust tests to changes in the existing functionalities'''
* '''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 ==
== Security ==
Line 81: Line 76:
* Input into a system should be checked for valid data size and range
* Input into a system should be checked for valid data size and range
* Avoid excessive logs for unusual behavior
* Avoid excessive logs for unusual behavior
-
* Consider purging highly sensitive from memory after use  
+
* Consider purging highly sensitive data from memory after use  
* Define wrappers around native methods (not declare a native method public)
* Define wrappers around native methods (not declare a native method public)
* Make public static fields final (to avoid caller changing the value)
* Make public static fields final (to avoid caller changing the value)

Latest revision as of 07:21, 19 April 2021

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).

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
Retrieved from "https://wiki.yadda.icm.edu.pl/yadda/Public:Code_Review"
Personal tools