From YaddaWiki

Revision as of 09:12, 16 November 2015

The Code Review Process in GitHub

Pull Requests in GitHub's Shared Repository Model. 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:

1. Create a feature branch: **_git checkout -b feature_branch_name_** (feature_branch_name should start with your github login)
2. Make your changes
3. Commit your changes
4. [Optionally, so the pull request will be more up-to-date] Clean up your history: **_git rebase master_**
5. Run tests: _**gradlew clean test**_ and _**gradlew clean slowTest**_
6. Push the changes: _**git push origin feature_branch_name**_
7. Issue a pull request on github (master as the base branch, feature as the head branch)
8. Wait for changes to be accepted. If you are asked to revise your changes, edit your branch and push the changes.
9. Clean up your history: **_git rebase master_**
10. Merge the feature branch into master: _**git checkout master**_ and then _**git merge feature_branch_name**_ (or **git merge --squash feature_branch_name**)
11. If merge was done with **--squash** option, then commit merged changes and modify commit message: _**git commit**_
12. Pull the changes from origin: _**git pull --rebase origin**_ (or git fetch origin and git rebase origin/master)
13. Run tests before push: _**gradlew clean test**_ and _**gradlew clean slowTest**_
14. Push the changes to the master branch: _**git push origin master**_
15. Delete the feature branch: _**git branch -D feature_branch_name**_ and then _**git push origin :feature_branch_name**_
16. [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/

• **Mind the project 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)!**
• **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 org.joda date library instead of standard java date 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**
• **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