Skip to content
Snippets Groups Projects

Check list_for_Code_Reviews

Last edited by Alexander Diewald

Code Reviews (CRs)

The code review system for AutoFOCUS 3 is file based and uses a traffic light system as mentioned in the Development Workflow.

  • RED: Not ready for review. Any reviewer rejects a CR request if not agreed otherwise before.
  • YELLOW: A file is considered “ready to be reviewed” by the developer that modified it.
  • GREEN: A file is considered compliant to the coding guidelines by the reviewer.

Code Reviews are done on the basis of Merge Requests (MRs). Details about the technical process to get a MR and upload the result of a CR are found in the Development Workflow.

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

For Developers: Turning the code from RED to YELLOW

  • Ensure that your code is warning free
  • Stick to the coding rules and conventions.
  • Moreover, especially when integrating source code that has been under development for a longer time, check if it is still based on the current code template
    • SVN Property tags ($Author$, $Id$, …) and @ConQAT tags should be removed
    • File header should start like this (check copyright date!) /*-------------------------------------------------------------------------+ | Copyright 20xx fortiss GmbH | | | | Licensed under the Apache License, Version 2.0 (the "License"); | [...]

Testing & Code Review: Turning the code from YELLOW to GREEN

  • General Rule: CRs have to be performed by a different person than the developer who has turned the code from RED to YELLOW (four-eyes principle).
  • Files not complying to the coding guidelines have to be marked RED. TODOs at the appropriate locations provide feedback to the original developer.

Instructions:

  1. First, test the MR: If several changes are needed to fix a discovered bug / misbehaviour the code review may need to be done several times, otherwise.
  2. If all is working as expected or new issues were created to fix the discovered problem, compare the code vs the master branch (see the slides at Development Workflow, Slide 23ff.).
  3. If you identify a violation of conventions or coding guidelines, write a comment “TODO (XX, k-j)” at the corresponding position in the code: XX denotes your initials, k-j denotes the number of the faulty item below.
  4. If you identify issues with the approach, mention the issue in the issue tracker when reassining the issue to the original developer (when the CR “iteration” if finished).
  5. Commit the files with remarks to be fixed with the comment “RED” that may be preceeded with a topic.
  6. Commit the files which are OK with the Heading “GREEN” that may be preceeded with a topic.
  7. Reassign the MR to the original developer.
  8. Reassign the Isuue to the original author if needed (comments about the approach, see above).
  9. when the code does not contain any issue, turn it to Green and commit it with the only comment “GREEN”

Coding Guidelines

The majority of code formatting issues is already resolved by our automatic code formatting rules. However, this is not sufficient such that the following guidelines have to be considered by ddevelopers when writing code and when a Reviewer has a look at this code.
Some of the violations of the coding style is checked by our tooling. Reviewers have to check the coding style only for those issues that are not detected by our tooling. Files without warnings are not allowed to get YELLOW/GREEN anyways.

Conventions for AF3

  • 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.
  • Test source and test models are located in the test-src and test-data folders of a plugin.

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

Style issues to be checked by a reviewer

# No Eclipse warning

# No unused (commented out) code

# No FIXME (indicate known bugs) or TODO without issue reference

# Comments:

## Comments for classes, attributes and methods start with a capital letter and finish with a dot

## Comments and methods do not contain any spelling mistake (Eclipse helps for comments)

## No empty comment (i.e., no comment without a content)

## 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.

## Method or attributes comments that fit one line should be one line, i.e., do not do:

    /**
     * blah.
     */

but:

<code class="java">/** blah. */

\#\# in comments always use {@link classname} when referencing a class

\# code readability:

\#\# 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.

\#\# the code is **concise**, e.g., length/depth of method should not be
too long; implement intermediate methods if necessary

\#\# 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

\#\# no magic constants are used, e.g.:

    <code class="java">int i = 43 * x

->what does 43 mean? use instead:

<code class="java">
final int multiplication_factor = 43;
int i = multiplication_factor * x;

  
similar:

    <code class="java">return -2

## warning may not be suppressed. only the following two exceptions apply:

- the suppressible warnings “rawtypes” and “unchecked” may be used everywhere in the code.

  • in migrators, deprecation warnings may be suppressed.

## use static imports for static methods (i.e., java.lang.utils.blah ->blah): mark method, ctrl+shift+m. can have exceptions for non-utility classes.

## when an expression spans two lines or more or is used several times, 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”)

## complementary, 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”)

  1. error handling: catches are never empty (at least a log message is sent to the console)
  2. classes and methods with similar functionality should be factorized

automatically detected style violations

# every method must be commented (but param/return do not have to be used systematically)

# 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)

# 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
  1. no useless empty lines (typically before a closing brace)
  2. big classes and methods should be split

standard patterns giving raise to improvements

the following can be explicitly mentioned during the code review by adding “todo (nn,cxx)” in the code.

  1. code having the form: if condition { return true; } else { return false; } 

      
    can/should be written:
        <code class="java">return condition;
(intermediate expressions might have to be introduced for the
condition, for readability)
  1. code having the form: if condition { x = true; } else { x = false; } 

      
    can/should be written:
        <code class="java">x = condition;
(intermediate expressions might have to be introduced for the
condition, for readability)
  1. code of the form: if (x < m ) { x = m ; } 

      
    should be written:
        <code class="javax = max(x, m);
(`max` is the available in the libray `java.lang.Math`)

Selectively disable 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.

ATTIC

Original code guideline list
Check-list:

  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)

    No empty comment (i.e., no comment without a content)

    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

    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

    The code is concise, e.g., length/depth of method should not be too long; implement intermediate methods if necessary

    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

    No magic constants are used, e.g.:

    "int i = 43 \* x" -> What does 43 mean? Use instead: "final int MULTIPLICATION\_FACTOR = 43; int i = MULTIPLICATION\_FACTOR \* x;" Similar: "return –2"

    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

    Warning may NOT be suppressed. Only the following two exceptions apply:

    • The suppressible warnings "rawtypes" and "unchecked" may be used everywhere in the code.
    • In migrators, deprecation warnings may be suppressed.

    Catches are never empty (at least a log message is sent to the console)

    If an issue is supposed to be fixed in the release, then no TODO should refer to it

    No useless empty lines (typically before a closing brace)

    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")

    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")

    In comments always use {`link class} when referencing a class

  8. Method or attributes comments that fit one line should be one line, i.e., do not do:
    /**
    @ * Blah.@
    @ */@
    But:
    /** Blah. */
  9. Big classes and methods should be split
  10. Classes and methods with similar functionality should be factorized
  11. Use static imports for static methods (i.e., java.lang.Utils.blah ->blah): Mark method, CTRL+SHIFT+M. Can have exceptions for non-utility classes.
  12. 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”)
  13. Comments should be concise (i.e., not explain the same thing twice, and avoid explaining something which is obvious, ex: “@param comp - The component”…)

Comments

    Please register or sign in to add a comment.