Check-list for Code Reviews

Whenever you perform review of AF3 code please pay attention to the following issues (before setting a file in status "YELLOW" or "GREEN"):

A. Turning the code from RED to YELLOW

As soon as you consider your code as being stable, you mark it as YELLOW.
YELLOW means "ready for review".

B. Turning the code from YELLOW to GREEN

This task is done by the code reviewer.
  1. read the code
  2. when you identify an issue matching one of the below items, write a comment "TODO (XX, k)" at the corresponding position in the code: XX denotes your initials, k denotes the number of the faulty item below
  3. commit the file with the comment "RED" (and only this)
  4. inform the author of the mistakes
  5. when the code does not contain any issue, turn it to Green and commit it with the only comment "GREEN"
  1. No unused (commented out) code
  2. No FIXME (indicate known bugs) or TODO without issue reference
  3. Every method must be commented (but @param/@return do not have to be used systematically)
  4. Comments for classes, attributes and methods start with a capital letter and finish with a dot
  5. Comments and methods do not contain any spelling mistake (Eclipse helps for comments)
  6. No Eclipse warning
  7. @param/@return should be used consistently: either not at all, or every parameter should have a corresponding @param and @return should never have an empty comment (but @param can have some empty comment)
  8. No empty comment (i.e., no comment without a content)
  9. The code can be easily understood (names are meaningful and homogeneous, no use of synonyms).
    If anything is suspect, take a closer look, ask the author
  10. The code is meaningfully commented (better no comment at all than a useless one), e.g., "Declaration of variable x." is not a meaningful comment
  11. The code is concise, e.g., length/depth of method should not be too long; implement intermediate methods if necessary
  12. The code reuses utility methods and does not re-implement again and again similar features:
    - The code implements similar features in a homogeneous way
    - Look pro-actively for reuse, do not hesitate to talk to other members
    - Good places to find reusable functionality are the "*.util" packages from each plugin
    - ConQAT commons packages also contain many useful methods
  13. No magic constants are used, e.g.:
    "int i = 43 * x"
    -> What does 43 mean? Use instead:
    "final int MULTIPLICATION_FACTOR = 43;
    Similar: "return -2"
  14. Using "get(0)" must be documented with an in-code comment stating why the "get(0)" is correct here, which means:
    - justify that the list is not empty
    - and justify that it is relevent to take the first element and not another
  15. Allowed suppressable warnings: "rawtypes", "unchecked" (i.e., other warnings are not suppressable)
  16. Catches are never empty (at least a log message is sent to the console)
  17. If an issue is supposed to be fixed in the release, then no TODO should refer to it
  18. No useless empty lines (typically before a closing brace)
  19. When an expression spans two lines or more, try using intermediate local variables to make it more readable
    (note: this can be easily done by 1. selecting the expression for which you want to define an intermediate local variable and 2. using the Eclipse menu "Refactor->Extract Local Variable")
  20. When an expression is used several times, introduce a local variable
    (note: this can be easily done by 1. selecting the expression for which you want to define an intermediate local variable and 2. using the Eclipse menu "Refactor->Extract Local Variable")
  21. In comments always use {@link class} when referencing a class
  22. Method or attributes comments that fit one line should be one line, i.e., do not do:
    * Blah.
    /** Blah. */
  23. Big classes and methods should be split
  24. Classes and methods with similar functionality should be factorized
  25. Use static imports (i.e., java.lang.Utils.blah -> blah). Can have exceptions for non-utility classes.
  26. No useless introduction of variables: if a variable is used only once and the corresponding expression fits on one line then just do not introduce the variable (note: removing such a variable can be easily done using the Eclipse menu "Refactor->Inline")
  27. Comments should be concise (i.e., not explain the same thing twice, and avoid explaining something which is obvious, ex: "@param comp - The component"...)

Standard patterns giving raise to improvements

The following can be explicitly mentioned during the code review by adding "TODO" in the code.
  1. Code having the form:
    if condition {
    return true;
    } else {
    return false;

    can/should be written:
    return condition ;
    (intermediate expressions might have to be introduced for the condition, for readability)
  2. Code having the form:
    if condition {
    x = true;
    } else {
    x = false;

    can/should be written:
    x = condition ;
    (intermediate expressions might have to be introduced for the condition, for readability)
  3. Code of the form:
    if ( x < m ){
    x = m ;

    Should be written:
    x = max( x , m );
    (max is the available in the libray java.lang.Math)

Code Reuse Guidelines

  1. Logging functionality should be done through LoggingUtils
  2. Assert functionality should be done through Eclipse.core.runtime.Assert
  3. UI Messages functionality should be done through MessageUtils
  4. "Should not happen" functionality should be done through LoggingUtils

Code formatting

If your code contains sections that would be destroyed by our Code-Formatter, you can use the following tags

  • // @CodeFormatterOff
  • // @CodeFormatterOn

to temporarily turn it off and then on again (if you are already in a comment context, you can of course omit the two slashes). Please do not forget to turn the Code-Formatter on again, i.e. do not switch the Code-Formatter off and never on again, as this could produce side effects for other developers.

Conventions for AF3 2.10

  • Constructors:
    • Omit public, no-argument constructor with empty implementation (its defined by Java by default if you have no other constructors).
    • Document constructor with /** Constructor. */. Document parameters if not derivable from class attributes or called super constructor.
  • Utility Classes and Methods:
    • are implemented in the utils package of a plugin, e.g., org.fortiss.af3.project.utils,
    • contain only static methods,
    • public methods must have their parameters and return value documented with @param and @return.
  • Model Element Factories:
    • are implemented in the utils package with the XYZModelElementFactory pattern (XYZ should refer to a package in the ECore meta-model), e.g., ComponentModelElementFactory, BehaviorModelElementFactory.
    • public methods should have their parameters documented with @param if the parameter is special in some way (e.g. name and comment parameter are not special).
    • EMF-generated factories must not be accessed from outside these utils facades.
  • Model attributes, which need casts, pickInstanceOf or filtered iteration, should be accessed by a method defined in the meta-model. That way the code can be easier understood. As an example refer to state.ecore in af3.state plugin. However, more complex computations (e.g. transitive closure computation of a graph) must go to utility classes.
  • Generated source is located in the generated-src folder of a plugin. Its content must not be edited manually or committed to the repository.
  • Test source and test models are located in the test-src and test-data folders of a plugin.

af3_review_examples.pdf - examples from the current code base (use Cmd-Shift-T to find the classes). (56.5 KB) Florian Hölzl, 02/05/2013 03:32 PM