From YaddaWiki

Revision as of 09:34, 16 November 2015 (view source) Lukdumi (Talk | contribs) ← Older edit		Revision as of 09:35, 16 November 2015 (view source) Lukdumi (Talk | contribs) Newer edit →
Line 1:		Line 1:
		+
	= The Code Review Process in GitHub =		= The Code Review Process in GitHub =

---

Revision as of 09:35, 16 November 2015

Contents 1 The Code Review Process in GitHub 1.1 Pull Requests in GitHub's Shared Repository Model 2 The Code Review Common Checklist 2.1 General /clean code/ 2.2 Exceptions 2.3 Tests 2.4 Security 2.5 Performance

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 follows:

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