...
- All code submissions should not degrade the quality of the policy/* project codebase.
- Any sonar, checkstyle issues must be fixed in the current review or a subsequent review.
- The same JIRA should be used for these submissions.
- API changes most likely will require updates to CSIT tests. It is the responsibility of the contributor to provide a code review to update/enhance the CSIT tests for their contribution.
...
Committer Strategy
- Require 3 committers to +1 a review - at least 2 companies represented.
- Require at least one expert of the repo to +1 (may or may not be a committer).
- For substantial reviews, either size or logic changes, then time limit of 24 hours to review can override.
- A +2 from committer or PTL, one can imply they are good with any future patches for a review.
Java Code Best Practices
In addition to adhering to ONAP sonar/checkstyle requirements, the project team has agreed upon these desired preferences for submitting java code.
- JUnit Tests
- Test file naming
- WIP
- "Test" prefix
- Where appropriate, use assertj instead of try/catch blocks. Try/catch blocks are notoriously incorrect or incorrectly maintained, as developers frequently forget to add "fail()" calls in the appropriate branch.
- WIP
- Test file naming
- Use of Static Methods
- In general, methods should not be declared "static" unless they are side-effect free. For instance, they should not interact with external systems (e.g., DBMS).
- Static methods should be short.
- Exceptions include "registration" methods, which are used to register objects once, typically at start-up. Loggers fall into this category.
- This is intended to be a guideline and not a hard and fast rule, though exceptions should be granted sparingly. Typically, a static method can be easily converted into a regular method.
- Rationale:
- Classes that invoke static methods are more difficult to test
- Limits code reuse: statics typically wrap singleton data structures. As a result, the class containing them cannot easily be reused in other contexts. Furthermore, static methods preclude behavior modification via sub-classing.
- Testing rainy day scenarios: It’s much easier to test rainy day scenarios if you can force an invoked method to throw an exception or otherwise return or modify values in some particular way. This is easier to accomplish if the method can be overridden.
- If the static method is in another class then powermock can be used to do this. If not, then it becomes problematic. This is true even when dealing with static methods that are side-effect free.
- Amount of configuration that must be done: To execute code that’s held together by static methods, sometimes lots of supporting classes have to be configured, when the use of mocks or partially configured objects would suffice if non-static methods were used.
- Starting tests fresh: When static methods manipulate static data, every test that uses those static methods, whether directly or indirectly, has to be aware that the static data may not have been left in a desired state. Typically, a mechanism must be added to re-initialize such data.
- Ease of running multiple objects in separate threads: When regular methods are used, it is typically quite easy to spin up multiple threads, each with its own object, when some type of multi-threaded testing must be done. That is often much more difficult if the objects use static methods that manipulate static data.
- Multiple types of junit tests: The first two, and sometimes the latter, as well, are made much easier using objects with regular methods.
- Individual classes/methods
- Code coverage
- Component tests
- If code contains numerous static methods, then that may be indicative of a design problem, possibly that code was broken down using a procedural programming style rather than an object-oriented style. While a procedural style may be appropriate in some cases, static methods used therein should be side-effect free.
- Numerous discussions on the web, but here are some:
- Amount of code a developer has to grasp to understand what's happening is larger with static methods: https://simpleprogrammer.com/static-methods-will-shock-you/
- Should be pure (i.e., side-effect free): https://programmingideaswithjake.wordpress.com/2016/04/02/static-methods-are-fine/
- Various other reasons found in: https://r.je/static-methods-bad-practice
- Classes that invoke static methods are more difficult to test
Committer Strategy
- Require 3 committers to +1 a review - at least 2 companies represented.
- Require at least one expert of the repo to +1 (may or may not be a committer).
- For substantial reviews, either size or logic changes, then time limit of 24 hours to review can override.
- A +2 from committer or PTL, one can imply they are good with any future patches for a review.