Skip to main content
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
Back to the top