Skip to main content
Jump to: navigation, search

CodeReviewChecklist

Revision as of 10:28, 23 October 2014 by Martin.grebac.oracle.com (Talk | contribs)

(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)

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