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: /** * blah. */ but: /** 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.: int i = 43 * x ->what does 43 mean? use instead: final int multiplication_factor = 43; int i = multiplication_factor * x; similar: 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: 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: