[Proposal] StreamPark code-style-and-quality Improve

[RocMarshal]

[VampireAchao]

The naming conventions are quite mixed up, the priority for variable names is to use nouns, and the priority for method names is to start with a verb.

For the db layer (non-service layer), the basic CRUD operations like getBy, selectBy, queryBy, etc., are now standardized according to com.baomidou.mybatisplus.core.mapper.BaseMapper:

• For querying records, use "select" as a prefix, like selectById, selectByXxx, selectPageByXxx.
• For database UPDATE operations, use "update" as a prefix.
• For database INSERT operations, use "insert" as a prefix.
• For database DELETE operations, use "delete" as a prefix.

For the service layer, basic CRUD operations are named according to com.baomidou.mybatisplus.extension.service.IService:

• For querying multiple records, use "list" as a prefix, like listByIds, listByXxx.
• For querying single records, use "get" as a prefix, like getByName, getOne.
• For database UPDATE operations, use "update" as a prefix.
• For database INSERT operations, use "save" as a prefix.
• For database DELETE operations, use "remove" as a prefix.

[VampireAchao]

For collection return values, unless there are special circumstances (such as needing thread safety), always return the interface.
○ For example, ArrayList returns List
○ HashMap returns Map
○ HashSet returns Set

[VampireAchao]

Redundant string encoding
org.apache.streampark.console.base.domain.RestResponse

  // 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();
    // strings not be extracted
    resp.put("status", STATUS_SUCCESS);
    resp.put("code", ResponseCode.CODE_SUCCESS);
    resp.put("data", data);
    return resp;
  }

[VampireAchao]

Unreasonable branch order results in multiple lines of code depth+1.
Generally speaking, if a method has a code line depth exceeding 2+ tabs (In Streampark, one TAB is equivalent to two spaces) due to nested if...else.., consider trying to merge branches, invert branch conditions, or extract private methods to reduce code line depth and enhance readability.

    if (request instanceof MultipartHttpServletRequest) {
      MultipartHttpServletRequest multipartRequest = (MultipartHttpServletRequest) request;
      Map<String, MultipartFile> files = multipartRequest.getFileMap();
      for (String file : files.keySet()) {
        MultipartFile multipartFile = multipartRequest.getFile(file);
        ApiAlertException.throwIfNull(
            multipartFile, "File to upload can't be null. Upload file failed.");
        boolean fileType = FileUtils.isJarFileType(multipartFile.getInputStream());
        ApiAlertException.throwIfFalse(
            fileType, "Illegal file type, Only support standard jar files. Upload file failed.");
      }
    }
    return true;


----->>


    if (!(request instanceof MultipartHttpServletRequest)) {
        return true;
    } 
    MultipartHttpServletRequest multipartRequest = (MultipartHttpServletRequest) request;
    Map<String, MultipartFile> files = multipartRequest.getFileMap();
    for (String file : files.keySet()) {
      MultipartFile multipartFile = multipartRequest.getFile(file);
      ApiAlertException.throwIfNull(
          multipartFile, "File to upload can't be null. Upload file failed.");
      boolean fileType = FileUtils.isJarFileType(multipartFile.getInputStream());
      ApiAlertException.throwIfFalse(
          fileType, "Illegal file type, Only support standard jar files. Upload file failed.");
    }
    return true;



--------------------

[VampireAchao]

Branch handling leads to redundant lines, making the code less concise.

org.apache.streampark.console.base.mybatis.interceptor.PostgreSQLQueryInterceptor#intercept

	CacheKey cacheKey;
    BoundSql boundSql;
    if (args.length == 4) {
      boundSql = ms.getBoundSql(parameter);
      cacheKey = executor.createCacheKey(ms, parameter, rowBounds, boundSql);
    } else {
      cacheKey = (CacheKey) args[4];
      boundSql = (BoundSql) args[5];
    }


->>

	boolean fourLen = args.length == 4;
    BoundSql boundSql = fourLen ? ms.getBoundSql(parameter) : (BoundSql) args[5];
    CacheKey cacheKey =
        fourLen ? executor.createCacheKey(ms, parameter, rowBounds, boundSql) : (CacheKey) args[4];
    

[VampireAchao]

if ... if (nested condition)

// map
      if (obj instanceof Map) {
        if (((Map<?, ?>) obj).isEmpty()) {
          return true;
        }
      }


-->>

	  // map
      if (obj instanceof Map  && ((Map<?, ?>) obj).isEmpty()) {
          return true;
      }

[VampireAchao]

too lines in a method

try to split the method body with mutil sub-methods at proper points.
1. for fine-test
2. for semantic
3. for fine-readable

[VampireAchao]

The method lacks comments：when, how, what , note

[VampireAchao]

The method's parameters and return values lack non-null or null annotations and constraints when there are restrictions.
○ For example, if a parameter cannot be null, it's best to add the "@nonnull" annotation.
○ For example, if a return value can be null, it's best to add the "@nullable" annotation to the method's implementation.

[VampireAchao]

Lacks necessary Class header documentation
○ Short-term solution: supplement with "what", "author", "note".
○ Long-term solution: After supplementing, add a class header documentation checker plugin (not to be done immediately).

[VampireAchao]

Lacks unit tests (final)
○ Basic components of unit tests #2896
○ service mock

[VampireAchao]

(Final) Splitting or refactoring of business logic
○ Whether the splitting is reasonable should be considered from the dimensions of components, logic, abstraction, etc.
○ In addition, attention should be paid to the verification and acquisition of check information for better testing.
○ VisableForTesting
○ Add unit tests as an aside.

[VampireAchao]

References：
https://flink.apache.org/zh/how-to-contribute/code-style-and-quality-preamble/
https://alibaba.github.io/p3c/
https://rules.sonarsource.com/java/

[RocMarshal]

Hi, @VampireAchao Thanks for your comments. LGTM & looking forward to the next step to advance it~ 👍

[RocMarshal]

• It would be better if the methods in the class could be sorted in the order of public, protected, private

[RocMarshal]

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/Commits.
   • 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:

// 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+ Tabs 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:

    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:

   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)

Note:

• The method declaration in the interface must be annotated.

• If the semantics of the implementation and the annotation content at the interface declaration are inconsistent, the specific implementation method also needs to be rewritten with annotations

• If the semantics of the method implementation are consistent with the annotation content at the interface declaration, it is not recommended to write annotations to avoid duplicate annotations

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:

    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);

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.

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:

       // 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:

       // 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/

[RocMarshal]

Hi, @VampireAchao Thanks for your efforts on the discussion page.
I make a draft on the existed comments. Could you help to take a look in your free time ?
Thank you~.

If there are no other new suggestions or changes before the end of this week, it would be the time to prepare to initiate a voting email.
CC @wolfboys @macksonmu @GOODBOY008

[wolfboys]

Great job, thanks for your driving and hard work. This is very helpful in improving the code quality of StreamPark. BTW, we can initiate another discussion about the scala code style

[wolfboys]

Add a rule: Enumeration type must have a suffix of "Enum", e.g: UserTypeEnum, ResourceTypeEnum.

[RocMarshal]

Add a rule: Enumeration type must have a suffix of "Enum", e.g: UserTypeEnum, ResourceTypeEnum.

Hi, @wolfboys Thanks for the suggestion~

Undoubtedly, the rule will improve readability. However, in some cases, it can make the declaration line of the enumeration type particularly long. This requires us to be as concise as possible when defining enumeration classes, and when using static methods referenced by enumerations, this issue can be magnified. Of course, in response to this issue, we can limit the use of static import methods when calling static methods to alleviate this problem.
The current IDE is already able to quickly know the type of a variable, and enumerating classes are not as complex as variables of type Map<>. It only represents the type, while map needs to reflect on the name that the variable maintains a mapping relationship. Perhaps I didn't express the difference very well, but it's like the definition of Map and enumeration. When defining the implementation class of Map, it needs to be declared with the class keyword, while the enumeration class needs to be declared with the enum keyword.

Of course, I'd like to hear more opinion/advice from you & others.

[RocMarshal]

Will be advanced in #2986

[RocMarshal]

Completed in #2986