世界上本来就有许多格格不入的事物为了共存而不得不相互接受。——博尔赫斯《沙之书》
最近在为streampark
起草代码规范指南,对应的pr
如下
https://github.com/apache/incubator-streampark-website/pull/226
对应的内容:
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
https://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
ISSUE
/PR
(pull request) driving and naming
PR
corresponds to ISSUE
.
Note: Hotfix
issue does not need to follow this rule, such as fixing spelling errors in JavaDoc
or document
files.
PR
, you can refer to the [ISSUE-XXXX][Feature/Improve/Refactor/Bug/Cleanup] Title
of the pull request,
where ISSUE-XXXX
should be replaced with the actual ISSUE
number.
PR
, such as new features, improvement, refactor, etc.PR
are within a certain module or component, they can be indicated in the commit message.PR
template to describe the contribution. So that the reviewer can understand the problem and solution from the description, rather than just from the code.PR
.ISSUE
, and most of the description is copied from there.PR
should be arranged changes such as Cleanup
, Refactor
, Improve
, and Feature
into separated PRs
/Commits
.PR
: [ISSUE-XXXX][Feature/Improve/Refactor/Cleanup] Title of the pull request
.
[ISSUE-xxxx1][Improve(ment)] Improve ...
[ISSUE-xxxx2][Refactor] Refactor ...
[ISSUE-xxxx3][Feature] Support ...
[ISSUE-xxxx4][Bug] Fix ...
[ISSUE-xxxx5][Feature][subtask] Support ...
[Hotfix][module_name] Fix xxx comments ...
Note: Try to use git history instead of annotated code (not mandatory)
spotless
Just run mvn spotless:apply
in the project repo root directory after installing the plugin.
checkstyle
Just run mvn checkstyle:checkstyle
after installing the plugin.
eslint
npx eslint --cache --max-warnings 0 "{src,mock}/**/*.{vue,ts,tsx}" --fix
npm run lint:eslint
Prioritize selecting nouns for variable naming, it’s easier to distinguish between variables
or methods
.
Cache<String> publicKeyCache;
Pinyin abbreviations are prohibited for variables (excluding nouns such as place names), such as chengdu.
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:
Map<Long, User> idUserMap;
Map<Long, String> userIdNameMap;
That can intuitively know the type and meaning of the variable through its name. Method names should start with a verb first as follows:
void computeVcores(Object parameter1);
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
update
insert
delete
The methods name of basic CRUD
of the service layer should be named as com.baomidou.mybatisplus.extension.service.IService
:
list
, such as listByIds
, listByXxx
getByName
and getOne
update
save
remove
Set the serialVersionUID
of all classes to 1L
, following Flink
’s serialVersionUID
.
Negative demo:
private static final long serialVersionUID = -8713837118340960775L;
Positive demo:
private static final long serialVersionUID = 1L;
Redundant strings should be extracted as constants
Negative demo:
public static RestResponse success(Object data) {
RestResponse resp = new RestResponse();
resp.put("status", "success");
resp.put("code", ResponseCode.CODE_SUCCESS);
resp.put("data", data);
return resp;
}
public static RestResponse error() {
RestResponse resp = new RestResponse();
resp.put("status", "error");
resp.put("code", ResponseCode.CODE_FAIL);
resp.put("data", null);
return resp;
}
Positive demo:
Strings are extracted as constant references.
public static final String STATUS_SUCCESS = "success";
public static final String STATUS_FAIL = "error";
public static final String STATUS = "status";
public static final String CODE = "code";
public static final String DATA = "data";
public static RestResponse success(Object data) {
RestResponse resp = new RestResponse();
resp.put(STATUS, STATUS_SUCCESS);
resp.put(CODE, ResponseCode.CODE_SUCCESS);
resp.put(DATA, data);
return resp;
}
public static RestResponse error() {
RestResponse resp = new RestResponse();
resp.put(STATUS, STATUS_FAIL);
resp.put(CODE, ResponseCode.CODE_FAIL);
resp.put(DATA, null);
return resp;
}
Variables that have not been reassigned must also be declared as final types.
public
, protected
, and private
@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
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.
For collection
returned values, unless there are special concurrent
(such as thread safety), always return the interface
, such as:
ArrayList
HashMap
HashSet
If there are multiple threads, the following declaration or returned types can be used:
private CurrentHashMap map;
public CurrentHashMap funName();
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.
Thread pool
needs to be resource constrained to prevent resource leakage caused by improper handling
condition/control
branches order leads to:
depths
of n+1
Generally speaking, if a method’s code line depth exceeds 2+ Tabs
due to continuous nested if... else..
, it should be considered to try
merging branches
,inverting branch conditions
extracting private methods
to reduce code line depth and improve readability like follows:
Union or merge the logic into the next level calling
Negative demo:
if (isInsert) {
save(platform);
} else {
updateById(platform);
}
Positive demo:
saveOrUpdate(platform);
Merge the conditions
Negative demo:
if (expression1) {
if(expression2) {
......
}
}
Positive demo:
if (expression1 && expression2) {
......
}
Reverse the condition
Negative demo:
public void doSomething() {
// Ignored more deeper block lines
// .....
if (condition1) {
...
} else {
...
}
}
Positive demo:
public void doSomething() {
// Ignored more deeper block lines
// .....
if (!condition1) {
...
return;
}
// ...
}
Using a single variable or method to reduce the complex conditional expression
Negative demo:
if (dbType.indexOf("sqlserver") >= 0 || dbType.indexOf("sql server") >= 0) {
...
}
Positive demo:
if (containsSqlServer(dbType)) {
....
}
//.....
// definition of the containsSqlServer
Using
sonarlint
andbetter highlights
to check code depth looks like good in the future.
When
: When can the method be calledHow
: How to use this method and how to pass parameters, etc.What
: What functions does this method achieveNote
: What should developers pay attention to when calling this methodWhat
, Note
, etc. like mentioned in the 1
.
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:
map.computeIfAbsent(key, x -> key.toLowerCase())
Positive demo:
map.computeIfAbsent(key, k -> k.toLowerCase());
Consider method references instead of inline lambdas
Negative demo:
map.computeIfAbsent(key, k-> Loader.load(k));
Positive demo:
map.computeIfAbsent(key, Loader::load);
Utils.requireXXX
to complete the validation of the prerequisite, and if possible, replace the AlertXXException.throwIfXXX
by new pre-conditions checking.This streampark-console-service
module is the core module for processing user requests.
It’s very necessary to strive to provide the best user experience.
So, we introduced the AbstractApiException
and its subclasses to get more friendly interaction effect. Non-AbstractApiException
is treated as internal server errors correspondingly, which needn’t notify the interaction details to users.
Based on the above premise, we need to pay attention to the handling of AbstractApiException
.
For example, we should throw an exception by one of followed subclasses of AbstractApiException
when processing logic with the user operation errors or missing data errors:
Failed to start job
, need to display the exception(stackTrace info) to front-end.
In addition to handling the classification of exceptions, we’d better make the precise and concise exception message and try to ensure the follows in the exception:
Please click Issue-2325 for more details about the items if needed.
Use placeholders
for log output:
Negative demo
log.info("Deploy cluster request " + deployRequest);
Positive demo
log.info("load plugin:{} to {}", file.getName(), appPlugins);
Pay attention to the selection of log level
when printing logs
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.
Negative demo:
Assuming the current log level is INFO
:
// ignored declaration lines.
List<User> userList = getUsersByBatch(1000);
LOG.debug("All users: {}", getAllUserIds(userList));
Positive demo:
In this case, we should determine the log level in advance before making actual log calls as follows:
// ignored declaration lines.
List<User> userList = getUsersByBatch(1000);
if (LOG.isDebugEnabled()) {
LOG.debug("All ids of users: {}", getAllIDsOfUsers(userList));
}
code/variables
used for testing
, you can use @VisableForTesting
annotation to indicate that
JUnit5
to develop test case preparation
AssertJ
to develop assertions statements.
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
.
real database
, environment or backend environment,
but doesn’t need to interact with external components, it’s recommended to inherit directly from SpringUnitTestBase
.
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
.
CI
overhead time too long and the resource load too heavy.