CodeReviewChecklist

Code Review Goals

• Spot and fix defects early in the process
• Better shared understanding of the codebase - learn from each other
• Identify common errors, reduce amount of work
• Build confidence in the overall quality of the solution/change
• Work and discuss together
• Assure the resulting product (code) adheres to best standards

Code Review Checklist

Security

• Is the code safe?
• Will it run/fail under security manager?
• Adding new calls for reflection, class loaders, doPrivileged ... ?

Findbugs

• Does the change introduce any new findbugs warnings?

Tests

• Is the change sufficiently covered by tests?
• Was enough testing done for the change?

Backwards Compatibility

• Does the change break backwards compatibility in any way?
• Does the code work on all supported JDKs? Databases? Servers?

Concurrency

• Does the code break under concurrency?
• Is it thread-safe? Should be?

Performance

• Any obvious performance anti-patterns?
• Do we understand performance implications of the change on performance - cpu&memory wise?
• Does it need new performance tests?

Maintenance

• Is there enough logging in the code?

Consistency

• E.g. - in case of tooling option changes - are we updating maven/ant plugins to include the options, too?
• Is the change consistent across the stacks? GF/WLS/..., ORADB/MSSQL/MYSQL/...
• Is the change made in moxy code consistent with jpa and vice versa?

Documentation, Javadoc

• Any new feature should be documented, what is not documented does not exist and is not done
• https://wiki.eclipse.org/EclipseLink/Development/DesignDocs
• Any new code should have good enough javadoc, API must have full comprehensive javadoc
• JDK 8 is more restrictive in javadoc - write javadoc jdk8 compatible

Legal

• Does the code have proper licence/copyright headers?
• Are copyright years updated?
• Is the code contributed properly - did the contributor really author 100% of the code? sign-off, ...

T13Y, I18N

• Make sure all messages and logging above or equal to level INFO is coming from resource bundles
• Follow proper I18N rules (do not combine bundles, ...)

Code style, Formatting

• Try to keep line-endings intact on unchanged lines/files
• Eclipse: $REPO/project-admin/EclipseLink-Eclipse-Format.xml
• http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html