[Discussion] Discussion on shenyu code suggestions

[yu199195]

Hi community

A project's code habits are very important,

especially for projects like ours that are entirely community-based.

So I'd like to invite everyone to come together and suggest better code to make the shenyu project better.

For example :

Null

• Ues Optional transform Null

Optional.ofNullable(xxx).orElse(xxxx)

• Ues Objects Judgment Null

Objects.isNull() or Objects.nonNull()

[impactCn]

I think when using the map list, if the length can be determined, specify the length as much as possible. so I think this is a good coding practice.

Use map as a container

• Map<> map = new HashMap<>(length)

  Use list as a container

• List<> list = new ArrayList<>(length)

[qinxu2015]

Some tools are available Hutool

[qicz]

Remove unnecessary else

        if (redirectHandle.getRedirectURI().startsWith(ROOT_PATH_PREFIX)) {
            ServerHttpRequest request = exchange.getRequest().mutate()
                    .uri(Objects.requireNonNull(UriUtils.createUri(redirectHandle.getRedirectURI()))).build();
            ServerWebExchange mutated = exchange.mutate().request(request).build();
            return dispatcherHandler.handle(mutated);
        } else {
            ServerHttpResponse response = exchange.getResponse();
            response.setStatusCode(HttpStatus.PERMANENT_REDIRECT);
            response.getHeaders().add(HttpHeaders.LOCATION, redirectHandle.getRedirectURI());
            return response.setComplete();
        }

Use local var replace more getter (shorter statements)

use "redirectURI" replace more "redirectHandle.getRedirectURI()"

        final String redirectURI = redirectHandle.getRedirectURI();
        if (redirectURI.startsWith(ROOT_PATH_PREFIX)) {
            ServerHttpRequest request = exchange.getRequest().mutate()
                    .uri(Objects.requireNonNull(UriUtils.createUri(redirectURI))).build();
            ServerWebExchange mutated = exchange.mutate().request(request).build();
            return dispatcherHandler.handle(mutated);
        } else {
            ServerHttpResponse response = exchange.getResponse();
            response.setStatusCode(HttpStatus.PERMANENT_REDIRECT);
            response.getHeaders().add(HttpHeaders.LOCATION, redirectURI);
            return response.setComplete();
        }

[SaberSola]

Constant values ​​are not allowed in the code like this checkEnable = Boolean.parseBoolean(System.getProperty("shenyu.upstream.check.enable", "false")); checkTimeout = Integer.parseInt(System.getProperty("shenyu.upstream.check.timeout", "3000")); healthyThreshold = Integer.parseInt(System.getProperty("shenyu.upstream.check.healthy-threshold", "1")); unhealthyThreshold = Integer.parseInt(System.getProperty("shenyu.upstream.check.unhealthy-threshold", "1")); checkInterval = Integer.parseInt(System.getProperty("shenyu.upstream.check.interval", "5000")); printEnable = Boolean.parseBoolean(System.getProperty("shenyu.upstream.check.print.enable", "true")); printInterval = Integer.parseInt(System.getProperty("shenyu.upstream.check.print.interval", "60000"));

[midnight2104]

Here is my view:

• The order of methods in a class: public > private . The following is not a good way.

 private ExecutorSubscriber getType(final List<DataTypeParent> list) {
       // ......
 }

public static class RegisterServerExecutorFactory extends AbstractQueueConsumerFactory {

    // ......
}

• Delete never used method. The following is not a good way.

    private String handlerTarsSelector(final MetaDataRegisterDTO metaDataDTO) {
        return getString(metaDataDTO);
    }

• Complex logical judgments can extract a constant to improve code readability. The following is not a good way.

    public ShenyuAdminResult applyCreate(final AuthApplyDTO authApplyDTO) {
        if (StringUtils.isBlank(authApplyDTO.getAppName())
                || (authApplyDTO.getOpen() && CollectionUtils.isEmpty(authApplyDTO.getPathList()))) {
            return ShenyuAdminResult.error(ShenyuResultMessage.PARAMETER_ERROR);
        }

        //......
    }

• Determine null values in advance to reduce cyclomatic complexity.The following is not a good way.

    public static void send(final String message, final DataEventTypeEnum type) {
        if (StringUtils.isNotBlank(message)) {
            if (DataEventTypeEnum.MYSELF == type) {
                Session session = (Session) ThreadLocalUtil.get(SESSION_KEY);
                if (session != null) {
                    sendMessageBySession(session, message);
                }
            } else {
                SESSION_SET.forEach(session -> sendMessageBySession(session, message));
            }
        }
    }

• If the method has too many parameters, a line break is recommended.The following is not a good way.

 @Override
    protected Mono<Void> doExecute(final ServerWebExchange exchange, final ShenyuPluginChain chain, final SelectorData selector, final RuleData rule) {
    //......
    }

[ttttangzhen]

• Avoid directly new RuntimeException, and use a custom Exception class to inherit RuntimeException

    public DynamicMessage parse(final InputStream inputStream) {
        try {
            //...
        } catch (IOException e) {
            throw new RuntimeException("Unable to merge from the supplied input stream", e);
        }
    }

• Avoid magic values ​​in the code

  public Mono<Void> execute(final ServerWebExchange exchange, final ShenyuPluginChain chain) {
      Boolean skip = Objects.requireNonNull(exchange.<Boolean>getAttribute("skip"));
      //return ...
  }

[li-keguo]

remove never used filed enviroment

@Configuration
public class HttpServerConfig {

    private final Environment environment;

    @Autowired
    public HttpServerConfig(final Environment environment) {
        this.environment = environment;
    }
// ...
}

I want to fix it

[JooKS-me]

All the unit tests are suggested to be a final class, as it is not likely to extend them.

Unit test class

define unit test class as a final class

public final class AesUtilsTest {
    ...
}

[yu199195]

General

• Use as many lambda function expressions as possible.

• Method parameters must be modified with final.

• Constants modified static final must be named in upper case.

• Methods should not have too many parameters.

• Clear unused classes and methods.

• Use linux line separators.

• Tab size is 4 and keep indents on empty lines.

• All code passage of Checkstyle: https://github.com/apache/incubator-shenyu/blob/master/script/shenyu_checkstyle.xml

Object

• Ues Optional transform Null.

Optional.ofNullable(xxx).orElse(obj)

• Ues Objects Judgment Null Or NotNull.

Objects.isNull(obj) OR Objects.nonNull(obj)

• Ues Objects.equals Judgment are they equal.

Objects.equals(obj1, obj2)

• Creater objects outside of a for loop.

Object object = null;
for () {
    object = new Object();
}

Collection

• must indicate initial capacity to avoid recalculate capacity.

List

• Use LinkedList when you need to add or delete elements, Else use ArrayList.

• Ues org.apache.commons.collections4.CollectionUtils Judgment Is empty Or Not empty.

CollectionUtils.isEmpty(list) or CollectionUtils.isNotEmpty(data)

Map

• Use ConcurrenHashMap when considering concurrency of threads, Else use HashMap

• Iterate over map using the most efficient way Or use lambda function.

Set<Map.Entry<String, String>> entrySet = map.entrySet();
Iterator<Map.Entry<String, String>> iter = entrySet.iterator();
while (iter.hasNext()) {
        Map.Entry<String, String> entry = iter.next();

}

String

• Ues org.apache.commons.lang3.StringUtils Judgment Is empty Or Not empty.

StringUtils.isEmpty(list) or StringUtils.isNotEmpty(data)

• String.join should be used when string concatenation occurs.

String join(CharSequence delimiter, CharSequence... elements)

Exception

• Do not use try...catch in a loop, it should be on the outermost layer.

try {
  for () {
  }
} catch () {

}

• Do not use printStackTrace().

• Please use custom exceptions or ShenyuException.

Resource

• Please use try with resource to close resource.