|
|
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](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](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](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:
|
|
|
|
|
|
|
|
|
```java">
|
|
|
/**
|
|
|
* 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:
|
|
|
<code class="java">
|
|
|
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)
|
|
|
2. code having the form:
|
|
|
<code class="java">
|
|
|
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)
|
|
|
3. code of the form:
|
|
|
<code class="java">
|
|
|
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”…)
|
|
|
|