EMF Compare/Reviews

From Eclipsepedia

Jump to: navigation, search

This is intended to serve as a quick check list for the code reviews, but it is not expected to be extensive and should be adapted when needed. A reviewer should not look for all items of this list at once, in a single review pass. Rather, he should be make one pass for each item, focusing only on that possible violation. Though the overall time to review will increase, it will make it much easier to spot the violations, reducing the maintenance cost in the long term.

Contents

Documentation

  • Does every Java file contain a copyright? Is it up to date? (See also the copyright guidelines.)
  • Is javadoc consistent with the code changes?

Code

  • Does the code completely implement the design?
  • Are there potential NPEs in the modified code?
  • Is there any array or list access with unchecked bounds (potential ArrayIndexOutOfBounds)?
  • Is there any 'switch' that may incorrectly fall through?
  • Is the code following the formatting and style rules of the project?
  • Any leftover stubs or tests in the code? Commented out lines or methods should be removed as well unless necessary (and documented).
  • Are there any unreachable branches or unneeded methods?
  • Can any method or predicate be replaced by reusable routines or library functions?
  • Any blocks of repeated code that could be externalized or compressed into a single method?
  • Are there undocumented magic numbers?
  • Is a method excessively complex? Could it be split into multiple, simpler routines?
  • Are all variables and methods properly defined with consistent and meaningful names?
  • Are there any unused or redundant variable?

Loops

  • Any loop that has a chance to run indefinitely?
  • Any loop that fails to iterate over _all_ elements (though foreach shouldn't be wrong, other for loops may have a wrong termination condition).
  • Are there any statements that could be pulled out of the loop body?

Arithmetic

  • Does the code use equality for floating point numbers (doubles and float)?
  • Are there any potential rounding error (float or double cast to short, int or long)?
  • Are divisions checked against division by zero?
  • Are floating-point numbers used in for loops? Check that they cannot go high enough for infinite loops (+1 has no effect on very large floating point numbers).

API

  • Any change in publicly accessible classes?
    • Should this really be public API?
    • Check that there are no API breaks.
    • Make sure that new methods or classes are documented with @since.

metadata

  • Is there any change of dependencies? Check the ranges.
  • Any new extension point?
    • Plugin must be a singleton.
    • exsd schema must be added to both source and binary builds (build.properties).
  • Any change in build.properties? Check that it was intended and will not break the actual build.
  • Any new plugin?
    • Must contain about.html
    • MANIFEST.MF should mention the Bundle-Localization entry. name and provider should be externalized. Avoid singleton directive when possible.
    • plugin.properties must contain accurate name and provider
    • build.properties must accurately point to about.html for both binary and source, as well as the MANIFEST and plugin.properties for binary.
    • Must contain a pom.xml and be referenced in the parent pom (org.eclipse.emf.compare-parent/pom.xml)