GitHub user RocMarshal edited a comment on the discussion: StreamPark
code-style-and-quality Improve
# StreamPark-console code-style-and-quality Improve
# 1 Pull Requests & Changes Rule
1. ISSUE/PR(pull request) driving and naming
- Ensure that PR corresponds to ISSUE.
> Note: Hotfix issue does not need to follow this rule, such as
fixing spelling errors in JavaDoc or document files.
- Naming format
When naming PR, you can refer to the
`[ISSUE-XXXX][Feature/Improve/Character/Cleanup] Title of the pull request`,
where `ISSUE-XXXX` should be replaced with the actual ISSUE number.
- These components should be the same as those used in original ISSUEs.
- The second part describes the type of PR, such as new features,
optimization, refactoring, etc. If all changes to PR are within a certain
module or component, they can be indicated in the `commit message`.
2. Description
- Please fill in the Pull Request template to describe the contribution. So
that the reviewer can understand the problem and solution from the description,
rather than just from the code.
- Ensure that the description is sufficient to illustrate the problem
addressed by the PR.
- Small changes do not require too much description.
- In an ideal scenario, the problem is described in ISSUE, and most of the
description is copied from there.
3. Try to break down changes into pure types of changes
- It's recommended that Pull requests should be arranged changes such as
`Cleanup`, `Refactor`, `Improve`, and `Feature` into separate `PRs/Commit`s.
- In this way, the Reviewers can independently view cleaning and
refactoring, and ensure that these changes do not change behavior.
- Then, the Reviewer can independently review the core changes and ensure
that they are a clean and robust change.
- In extreme cases, if a rollback commit is required, it can provide the
optimal granularity for version rollback selection.
- In addition, significant contributions should be split into a set of
independent changes that can be reviewed independently.
4. Commit message naming
The commit of messages should follow a pattern similar to the PR:
`[ISSUE-XXXX][Feature/Improve/Refactor/Cleanup] Title of the pull request`
- [Hotfix][module_name] Fix xxx comments
- [ISSUE-xxxx1][Improvement] Improve
- [ISSUE-xxxx2][Refactor] Refactor
- [ISSUE-xxxx2][Feature] Support
- [ISSUE-xxxx3][Feature][subtask] Support
> Note: Try to use git history instead of annotated code (not mandatory)
# 2 Code Checkstyle
- Backend code formatting Maven plugin: `spotless`
Just run `mvn spotless:apply` in the project repo root directory after
installing the plugin.
- Code specification Maven plugin: `checkstyle`
Just run `mvn checkstyle:checkstyle` after installing the plugin.
- Front end code formatting plugin `eslint`
- The original command is `npx eslint --cache --max-warnings 0
"{src,mock}/**/*.{vue,ts,tsx}" --fix`
- Encapsulated as `npm run lint:eslint`
# 3 Programming Specification
## 3.1 Naming Style
1. Prioritize selecting nouns for variable naming
It's easier to distinguish between variables or methods
- Example of variables:
`Cache<String> publicKeyCache;`
2. Pinyin abbreviations are prohibited for variables (excluding nouns such as
place names)
Such as chengdu.
3. It is recommended to end variable names with a type
For variables of type Collection/List, take `xxxx` (plural representing
multiple elements) or end with `xxxList` (specific type)
For variables of type map, describe the key and value clearly:
Example:
- `Map<Long, User> idUserMap`
- `Map<Long, String> userIdNameMap`
That can intuitively know the type and meaning of the variable through its
name
4. Method names should start with a verb first as follows:
- `void computeVcores(Object parameter1);`
5. The methods name of basic CRUD of the database layer (non-service layer)
should be uniformly standardized according to name
`com.baomidou.mybatisplus.core.mapper.BaseMapper`:
- If perform a database `select` operation, the name of the method should be
started with `select`
For example, `selectById`, `selectByXxx`, `selectPageByXxx`
- If perform a database `update` statement operation, the name of the method
should be started with `update`
- If perform a database `insert` statement operation, the name of the method
should be started with `insert`
- If perform a database `delete` statement operation, the name of the method
should be started with `delete`
6. The methods name of basic CRUD of the service layer should be named as
`com.baomidou.mybatisplus.extension.service.IService`:
- If perform a database `select operation to query multiple records`, the
name of the method should be started with a `list`, such as `listByIds`,
`listByXxx`
- If perform a database `select operation to query a single record`, the name
of the method should be started with `get`, such as `getByName` and `getOne`
- If perform a database `update` operation, the name of the method should be
started with `update`
- If perform a database `insert` operation, the name of the method should be
started with `save`
- If perform a database `delete` operation, the name of the method should be
started with `remove`
## 3.2 Constant Definition
1. Set the serialVersionUID of all classes to 1L, following Flink's
serialVersionUID
2. Redundant strings should be modified to constants, here are a positive demo
and a negative demo:
```java
// Positive demo: Strings are extracted as constant references.
public static final String STATUS_SUCCESS = "success";
public static final String STATUS_FAIL = "error";
private static final long serialVersionUID = -8713837118340960775L;
public static RestResponse success(Object data) {
RestResponse resp = new RestResponse();
// Negative demo：Strings not be extracted
resp.put("status", STATUS_SUCCESS);
resp.put("code", ResponseCode.CODE_SUCCESS);
resp.put("data", data);
return resp;
}
```
3. Variables that have not been reassigned must also be declared as `final`
types
## 3.3 Methods Rule
1. Sort the methods in the class in the order of `public`, `protected`, and
`private`
2. When there are restrictions on the method, the parameters and `returned`
values of the method need to be annotated with `NonNull` or `Nullable`
annotations and constraints.
For example, if the parameter cannot be `null`, it is best to add a
`@NonNull` annotation. If the `returned` value can be `null`, the `@Nullable`
annotation should be added first.
> Note: that the package name is `javax.validation.requirements`
3. If there are too many lines of code in the method, please have a try on
using multiple sub methods at appropriate points to segment the method body
Generally speaking, it needs to adhere to the following principles:
- Convenient testing
- Good semantics
- Easy to read
In addition, it is also necessary to consider whether the splitting is
reasonable in terms of components, logic, abstraction, and other aspects in the
scenario
> However, there is currently no clear definition of demo. During the evolution
> process, we will provide additional examples for developers to have a clearer
> reference and understanding.
## 3.4 Collection Rule
For `collection` returned values, unless there are special concurrent (such as
thread safety), always return the interface, such as:
- returns `List` if use `ArrayList`
- returns `Map` if use `HashMap`
- returns `Set` if use `HashSet`
If there are multiple threads, the following declaration or returned types can
be used:
- `private CurrentHashMap map;`
- `public CurrentHashMap funName();`
## 3.5 Concurrent Processing
1. The thread pool needs to be managed, using a unified entry point to obtain
the thread pool
> Note: During the evolution process, we will provide additional examples
for developers to have a clearer reference and understanding.
2. Thread pools need to be resource constrained to prevent resource leakage
caused by improper handling。
## 3.6 Control/Condition Statements
1. Unreasonable condition/control branches order leads to
- multiple code line depths of `n+1`
- redundant lines
Generally speaking, if a method's code line depth exceeds `2+` `Tab`s
due to continuous nested `if... else..`, it should be considered to try merging
branches, inverting branch conditions, or extracting private methods to reduce
code line depth and improve readability.
Using `sonarlint` and `better highlights` to check code depth looks
like good in the future.
- Demo:
```java
if (isInsert) {
save(platform);
} else {
updateById(platform);
}
// [Union or merge the logic into the next level calling] The lines could
be optimized as:
saveOrUpdate(platform);
// ------ The next demo split line-------
if (expression1) {
if(expression2) {
......
}
}
// [Merge the condition] The lines could be optimized as:
if (expression1 && expression2) {
......
}
// ------ The next demo split line-------
public void doSomething() {
// Ignored more deeper block lines
// .....
if (condition1) {
...
} else {
...
}
}
// [Reverse the condition] The lines could be optimized as:
public void doSomething() {
// Ignored more deeper block lines
// .....
if (!condition1) {
...
return;
}
...
}
```
2. Complex conditional expressions should be named using a single variable or
method as much as possible.
For example:
```java
if (dbType.indexOf("sqlserver") >= 0 || dbType.indexOf("sql server") >= 0) {
...
}
// [Extract it as a new condition getter] The lines could be refactored as:
if (containsSqlServer(dbType)) {
...
}
// definetion of the containsSqlServer ....
```
## 3.7 Code Comments Rule
1. Method lacks comments:
- `When`: When can the method be called
- `How`: How to use this method and how to pass parameters, etc.
- `What`: What functions does this method achieve
- `Note`: What should developers pay attention to when calling this method
2. Missing necessary class header description comments.
- Short term solution: Add `What`, `Note` like mentioned in the `1.`
- Long term solution: After supplementation, add a header
description/comments checker plugin to check automatically (not completed
immediately)
## 3.8 Java Lambdas
- Prefer non-capturing lambdas (lambdas that do not contain references to the
outer scope). Capturing lambdas need to create a new object instance for every
call. Non-capturing lambdas can use the same instance for each invocation.
- Negative demo:
```java
map.computeIfAbsent(key, x -> key.toLowerCase())
```
- Positive demo:
```java
map.computeIfAbsent(key, k -> k.toLowerCase());
```
- Consider method references instead of inline lambdas
- Negative demo:
```java
map.computeIfAbsent(key, k-> Loader.load(k));
```
- Positive demo:
```java
map.computeIfAbsent(key, Loader::load);
```
## 3.9 Java Streams
- Avoid Java Streams in any performance critical code.
- The main motivation to use Java Streams would be to improve code readability.
As such, they can be a good match in parts of the code that are not
data-intensive, but deal with coordination..
- Even in the latter case, try to limit the scope to a method, or a few private
methods within an internal class.
## 3.10 Pre-Conditions Checking
Use a unified `Utils.requireXXX` to complete the validation of the
prerequisite, and if possible, replace the `AlertXXException.throwIfXXX` by
new pre-conditions checking.
# 4 Exception Processing
It could reference to [About Exception Processing for
streampark-console-service
Module](https://streampark.apache.org/community/submit_guide/items-need-to-note.md/#about-exception-processing-for-streampark-console-service-module).
# 5 Log
1. Use placeholders for log output:
- The negative example: `log.info("Deploy cluster request " +
deployRequest);`
- The positive example: `log.info("load plugin:{} to {}", file.getName(),
appPlugins);`
2. Pay attention to the selection of log level when printing logs
3. When printing the log content, if the actual parameters of the log
placeholder are passed, it is necessary to avoid premature evaluation to avoid
unnecessary evaluation caused by the log level.
The negative example: Assuming the current log level is `INFO`:
```java
// ingnored declaration lines.
List<User> userList = getUsersByBatch(1000);
LOG.debug("All users: {}", getAllUserIds(userList));
```
In this case, we should determine the log level in advance before making
actual log calls as follows:
```java
// ingnored declaration lines.
List<User> userList = getUsersByBatch(1000);
if (LOG.isDebugEnabled()) {
LOG.debug("All ids of users: {}", getAllIDsOfUsers(userList));
}
```
# 6 Testing
1. For some of the `code / variables` used for testing, you can use
`@VisableForTesting` annotation to indicate that
2. It's recommended to use JUnit5 to develop test case preparation
3. Using `AssertJ` to develop assertions statements.
4. About the implementation of tests.
- If the test case only tests an independent class or method that does not
require external components such as `hadoop`, `remote flink session cluster`,
etc., it can be written directly using `JUnit5` & `Mockito`.
- If the test case needs a real database, environment or backend
environment, but doesn't need to interact with external components, it's
recommended to inherit directly from `SpringUnitTestBase`.
- If the test case requires a real database, environment or backend
environment, but needs to interact with external components (`Remote Flink
session cluster`, `Hadoop cluster`), it's recommended to write the test case by
directly inheriting `SpringIntegrationTestBase`.
5. It's only recommended to use integration tests on critical test links to
avoid making the `CI` overhead time too long and the resource load too heavy.
# How to do
- Introduce integration test classes and tools
- Add this document to the official website in the appropriate place.
- Pick the `streampark-console-service` package as the main scope of
refactoring, and make changes according to the documentation.
- Ideally, refactor the code quality at a granularity of the package scope at
all leaf node locations in the module.
- It is desirable to introduce the necessary test cases.
- After the above there items are completed, gradually add the description and
check of the Class header, and add the method comments in the interface
declaration class.
- Introduce the `description/comments` check plugin.
- Try to apply these rules to other modules
# References
- https://site.mockito.org/
- https://flink.apache.org/zh/how-to-contribute/code-style-and-quality-preamble/
- https://alibaba.github.io/p3c/
- https://rules.sonarsource.com/java/
- https://streampark.apache.org/community/submit_guide/items-need-to-note.md
- https://joel-costigliola.github.io/assertj/index.html
- https://junit.org/junit5/
GitHub link:
https://github.com/apache/incubator-streampark/discussions/2915#discussioncomment-6793456
----
This is an automatically sent email for [email protected].
To unsubscribe, please send an email to: [email protected]
