From 088bcf340437d337ffc169567f6bdfaad3a6d2d6 Mon Sep 17 00:00:00 2001 From: AB Date: Mon, 15 Sep 2025 14:54:51 +0200 Subject: [PATCH 1/7] Add PMD 7.16 Rules --- .config/pmd/java/ruleset.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index 267fa5e9..5d76b2b6 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -153,6 +153,8 @@ + + From 108486865e6442363bbcaff9b394ba5b36a2d63f Mon Sep 17 00:00:00 2001 From: AB Date: Mon, 15 Sep 2025 15:03:03 +0200 Subject: [PATCH 2/7] PMD: Import and modify rules from `jPinpoint` See https://github.com/jborgers/PMD-jPinpoint-rules --- .config/pmd/java/ruleset.xml | 824 ++++++++++++++++++++++++++++++++++- 1 file changed, 818 insertions(+), 6 deletions(-) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index 5d76b2b6..28bc272e 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -11,7 +11,6 @@ - @@ -199,6 +198,33 @@ + + + + Usually all cases where `StringBuilder` (or the outdated `StringBuffer`) is used are either due to confusing (legacy) logic or may be replaced by a simpler string concatenation. + + Solution: + * Do not use `StringBuffer` because it's thread-safe and usually this is not needed + * If `StringBuilder` is only used in a simple method (like `toString`) and is effectively inlined: Use a simpler string concatenation (`"a" + x + "b"`). This will be optimized by the Java compiler internally. + * In all other cases: + * Check what is happening and if it makes ANY sense! If for example a CSV file is built here consider using a proper library instead! + * Abstract the Strings into a DTO, join them together using a collection (or `StringJoiner`) or use Java's Streaming API instead + + 3 + + + + + + + + + - @@ -236,7 +262,7 @@ - @@ -257,7 +283,7 @@ - @@ -279,7 +305,7 @@ - @@ -303,7 +329,7 @@ - @@ -311,4 +337,790 @@ + + + + + + Do not use native HTML! Use Vaadin layouts and components to create required structure. + If you are 100% sure that you escaped the value properly and you have no better options you can suppress this. + + 2 + + + + + + + + + + + + + + + + java.text.NumberFormat: DecimalFormat and ChoiceFormat are thread-unsafe. + + Solution: Create a new local one when needed in a method. + + 1 + + + + + + + + + + + + + + + A regular expression is compiled implicitly on every invocation. + Problem: This can be (CPU) expensive, depending on the length of the regular expression. + + Solution: Compile the regex pattern only once and assign it to a private static final Pattern field. + java.util.Pattern objects are thread-safe, so they can be shared among threads. + + 2 + + + + 5 and +(matches(@Image, '[\.\$\|\(\)\[\]\{\}\^\?\*\+\\]+'))) +or +self::VariableAccess and @Name=ancestor::ClassBody[1]/FieldDeclaration/VariableDeclarator[StringLiteral[string-length(@Image) > 5 and +(matches(@Image, '[\.\$\|\(\)\[\]\{\}\^\?\*\+\\]+'))] or not(StringLiteral)]/VariableId/@Name] +]]> + + + + + + + + + + + + The default constructor of ByteArrayOutputStream creates a 32 bytes initial capacity and for StringWriter 16 chars. + Such a small buffer as capacity usually needs several expensive expansions. + + Solution: Explicitly declared the buffer size so that an expansion is not needed in most cases. + Typically much larger than 32, e.g. 4096. + + 2 + + + + + + + + + + + + + + + The time to find element is O(n); n = the number of enum values. + This identical processing is executed for every call. + Considered problematic when `n > 3`. + + Solution: Use a static field-to-enum-value Map. Access time is O(1), provided the hashCode is well-defined. + Implement a fromString method to provide the reverse conversion by using the map. + + 3 + + + + 3]//MethodDeclaration/Block + //MethodCall[pmd-java:matchesSig('java.util.stream.Stream#findFirst()') or pmd-java:matchesSig('java.util.stream.Stream#findAny()')] + [//MethodCall[pmd-java:matchesSig('java.util.stream.Stream#of(_)') or pmd-java:matchesSig('java.util.Arrays#stream(_)')] + [ArgumentList/MethodCall[pmd-java:matchesSig('_#values()')]]] +]]> + + + + + fromString(String name) { + return Stream.of(values()).filter(v -> v.toString().equals(name)).findAny(); // bad: iterates for every call, O(n) access time + } +} + +Usage: `Fruit f = Fruit.fromString("banana");` + +// GOOD +public enum Fruit { + APPLE("apple"), + ORANGE("orange"), + BANANA("banana"), + KIWI("kiwi"); + + private static final Map nameToValue = + Stream.of(values()).collect(toMap(Object::toString, v -> v)); + private final String name; + + Fruit(String name) { this.name = name; } + @Override public String toString() { return name; } + public static Optional fromString(String name) { + return Optional.ofNullable(nameToValue.get(name)); // good, get from Map, O(1) access time + } +} +]]> + + + + + + A regular expression is compiled on every invocation. + Problem: this can be expensive, depending on the length of the regular expression. + + Solution: Usually a pattern is a literal, not dynamic and can be compiled only once. Assign it to a private static field. + java.util.Pattern objects are thread-safe so they can be shared among threads. + + 2 + + + + + + + + + + + + + + + + Recreating a DateTimeFormatter is relatively expensive. + + Solution: Java 8+ java.time.DateTimeFormatter is thread-safe and can be shared among threads. + Create the formatter from a pattern only once, to initialize a static final field. + + 2 + + + + + + + + + + + + Creating a security provider is expensive because of loading of algorithms and other classes. + Additionally, it uses synchronized which leads to lock contention when used with multiple threads. + + Solution: This only needs to happen once in the JVM lifetime, because once loaded the provider is typically available from the Security class. + Create the security provider only once: Only in case when it's not yet available from the Security class. + + 2 + + + + + + + + + + + + + + + Reflection is relatively expensive. + + Solution: Avoid reflection. Use the non-reflective, explicit way like generation by IDE. + + 2 + + + + + + + + + + + + + + + java.util.SimpleDateFormat is thread-unsafe. + The usual solution is to create a new one when needed in a method. + Creating SimpleDateFormat is relatively expensive. + + Solution: Use java.time.DateTimeFormatter. These classes are immutable, thus thread-safe and can be made static. + + 2 + + + + + + + + + + + + + + + Creating Comparator instances repeatedly in methods like compareTo or sort calls is inefficient. + + Solution: Initialize the Comparator once as a static final field and reuse. + + 2 + + + + + + + + + { + @Override + public int compareTo(@NotNull Person o) { + return Comparator.comparing(Person::getFirstName) // Bad: Creates new Comparator instance on each invocation + .thenComparing(Person::getLastName) + .thenComparingInt(Person::getAge) + .compare(this, o); + } +} + +public class GoodPerson implements Comparable { + private static final Comparator COMPARE_FIRST_LAST_NAME_AGE = + Comparator.comparing(Person::getFirstName) + .thenComparing(Person::getLastName) + .thenComparingInt(Person::getAge); + + @Override + public int compareTo(@NotNull Person o) { + return COMPARE_FIRST_LAST_NAME_AGE.compare(this, o); // Good: Comparator initialized once as static final field + } +} +]]> + + + + + + Blocking calls, for instance remote calls, may exhaust the common pool for some time thereby blocking all other use of the common pool. + In addition, nested use of the common pool can lead to deadlock. Do not use the common pool for blocking calls. + The parallelStream() call uses the common pool. + + Solution: Use a dedicated thread pool with enough threads to get proper parallelism. + The number of threads in the common pool is equal to the number of CPUs and meant to utilize all of them. + It assumes CPU-intensive non-blocking processing of in-memory data. + + 2 + + + + + + + + + list = new ArrayList(); + final ForkJoinPool myFjPool = new ForkJoinPool(10); + final ExecutorService myExePool = Executors.newFixedThreadPool(10); + + void bad1() { + list.parallelStream().forEach(elem -> storeDataRemoteCall(elem)); // bad + } + + void good1() { + CompletableFuture[] futures = list.stream().map(elem -> CompletableFuture.supplyAsync(() -> storeDataRemoteCall(elem), myExePool)) + .toArray(CompletableFuture[]::new); + CompletableFuture.allOf(futures).get(10, TimeUnit.MILLISECONDS)); + } + + void good2() throws ExecutionException, InterruptedException { + myFjPool.submit(() -> + list.parallelStream().forEach(elem -> storeDataRemoteCall(elem)) + ).get(); + } + + String storeDataRemoteCall(String elem) { + // do remote call, blocking. We don't use the returned value. + RestTemplate tmpl; + return ""; + } +} +]]> + + + + + + Future.supplyAsync is typically used for remote calls. By default, it uses the common pool. + The number of threads in the common pool is equal to the number of CPU's, which is suitable for in-memory processing. + For I/O, however, this number is typically not suitable because most time is spent waiting for the response and not in CPU. + The common pool must not be used for blocking calls. + + Solution: A separate, properly sized, pool of threads (an Executor) should be used for the async calls. + + 2 + + + + + + + + +>[] futures = accounts.stream() + .map(account -> CompletableFuture.supplyAsync(() -> isAccountBlocked(account))) // bad + .toArray(CompletableFuture[]::new); + } + + void good() { + CompletableFuture>[] futures = accounts.stream() + .map(account -> CompletableFuture.supplyAsync(() -> isAccountBlocked(account), asyncPool)) // good + .toArray(CompletableFuture[]::new); + } +} +]]> + + + + + + `take()` stalls indefinitely in case of hanging threads and consumes a thread. + + Solution: use `poll()` with a timeout value and handle the timeout. + + 2 + + + + + + + + + void collectAllCollectionReplyFromThreads(CompletionService> completionService) { + try { + Future> futureLocal = completionService.take(); // bad + Future> futuresGood = completionService.poll(3, TimeUnit.SECONDS); // good + responseCollector.addAll(futuresGood.get(10, TimeUnit.SECONDS)); // good + } catch (InterruptedException | ExecutionException e) { + LOGGER.error("Error in Thread : {}", e); + } catch (TimeoutException e) { + LOGGER.error("Timeout in Thread : {}", e); + } +} +]]> + + + + + + Stalls indefinitely in case of stalled Callable(s) and consumes threads. + + Solution: Provide a timeout to the invokeAll/invokeAny method and handle the timeout. + + 2 + + + + + + + + +> executeTasksBad(Collection> tasks, ExecutorService executor) throws Exception { + return executor.invokeAll(tasks); // bad, no timeout + } + private List> executeTasksGood(Collection> tasks, ExecutorService executor) throws Exception { + return executor.invokeAll(tasks, OUR_TIMEOUT_IN_MILLIS, TimeUnit.MILLISECONDS); // good + } +} +]]> + + + + + + Stalls indefinitely in case of hanging threads and consumes a thread. + + Solution: Provide a timeout value and handle the timeout. + + 2 + + + + + + + + + complFuture) throws Exception { + return complFuture.get(); // bad +} + +public static String good(CompletableFuture complFuture) throws Exception { + return complFuture.get(10, TimeUnit.SECONDS); // good +} +]]> + + + + + + + Apache HttpClient with its connection pool and timeouts should be setup once and then used for many requests. + It is quite expensive to create and can only provide the benefits of pooling when reused in all requests for that connection. + + Solution: Create/build HttpClient with proper connection pooling and timeouts once, and then use it for requests. + + 3 + + + + + + + + + connectBad(Object req) { + HttpEntity requestEntity = new HttpEntity<>(req); + + HttpClient httpClient = HttpClientBuilder.create().setMaxConnPerRoute(10).build(); // bad + return remoteCall(httpClient, requestEntity); + } +} +]]> + + + + + + Problem: Gson creation is relatively expensive. A JMH benchmark shows a 24x improvement reusing one instance. + + Solution: Since Gson objects are thread-safe after creation, they can be shared between threads. + So reuse created instances from a static field. + Pay attention to use thread-safe (custom) adapters and serializers. + + 3 + + + + + + + + + + + From deddd914be12567ecf33146f9a2f7acb0c10fa0a Mon Sep 17 00:00:00 2001 From: AB Date: Tue, 16 Sep 2025 14:36:18 +0200 Subject: [PATCH 3/7] PMD: Exclude unused rule --- .config/pmd/java/ruleset.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index 28bc272e..517b9243 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -185,6 +185,9 @@ + + + From 53b1e5dfd3fc70d91302cfb4fef2dd48b1b65ccb Mon Sep 17 00:00:00 2001 From: AB Date: Tue, 16 Sep 2025 14:36:48 +0200 Subject: [PATCH 4/7] PMD: Reword and also apply to runAsync --- .config/pmd/java/ruleset.xml | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index 517b9243..cda412b8 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -841,6 +841,8 @@ public class GoodPerson implements Comparable { Solution: Use a dedicated thread pool with enough threads to get proper parallelism. The number of threads in the common pool is equal to the number of CPUs and meant to utilize all of them. It assumes CPU-intensive non-blocking processing of in-memory data. + + See also: [_Be Aware of ForkJoinPool#commonPool()_](https://dzone.com/articles/be-aware-of-forkjoinpoolcommonpool) 2 @@ -897,22 +899,25 @@ public class Foo { - Future.supplyAsync is typically used for remote calls. By default, it uses the common pool. + CompletableFuture.supplyAsync/runAsync is typically used for remote calls. + By default it uses the common pool. The number of threads in the common pool is equal to the number of CPU's, which is suitable for in-memory processing. For I/O, however, this number is typically not suitable because most time is spent waiting for the response and not in CPU. The common pool must not be used for blocking calls. - Solution: A separate, properly sized, pool of threads (an Executor) should be used for the async calls. + Solution: A separate, properly sized pool of threads (an Executor) should be used for the async calls. + + See also: [_Be Aware of ForkJoinPool#commonPool()_](https://dzone.com/articles/be-aware-of-forkjoinpoolcommonpool) 2 From e6358ed1fa1e6c743e9a7b055a55862beacb8677 Mon Sep 17 00:00:00 2001 From: AB Date: Tue, 16 Sep 2025 14:36:56 +0200 Subject: [PATCH 5/7] PMD: Fix error --- .config/pmd/java/ruleset.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index cda412b8..bb8177f4 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -1066,7 +1066,7 @@ public static String good(CompletableFuture complFuture) throws Exceptio //MethodDeclaration//MethodCall[ pmd-java:matchesSig('org.apache.hc.client5.http.impl.classic.HttpClientBuilder#create()') or pmd-java:matchesSig('org.apache.hc.client5.http.impl.classic.HttpClients#custom()') - or pmd-java:matchesSig('org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder#build()' + or pmd-java:matchesSig('org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder#build()') ] [ancestor::MethodDeclaration//ClassType[pmd-java:typeIs('org.springframework.http.HttpEntity') or pmd-java:typeIs('org.springframework.http.ResponseEntity')] From 042b7a495804644325788e8d174aa2d02622da85 Mon Sep 17 00:00:00 2001 From: AB Date: Tue, 16 Sep 2025 14:37:10 +0200 Subject: [PATCH 6/7] PMD: Remove rule as it yields too many FP --- .config/pmd/java/ruleset.xml | 63 ------------------------------------ 1 file changed, 63 deletions(-) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index bb8177f4..a5b76341 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -766,69 +766,6 @@ public class Foo { - - - Creating Comparator instances repeatedly in methods like compareTo or sort calls is inefficient. - - Solution: Initialize the Comparator once as a static final field and reuse. - - 2 - - - - - - - - - { - @Override - public int compareTo(@NotNull Person o) { - return Comparator.comparing(Person::getFirstName) // Bad: Creates new Comparator instance on each invocation - .thenComparing(Person::getLastName) - .thenComparingInt(Person::getAge) - .compare(this, o); - } -} - -public class GoodPerson implements Comparable { - private static final Comparator COMPARE_FIRST_LAST_NAME_AGE = - Comparator.comparing(Person::getFirstName) - .thenComparing(Person::getLastName) - .thenComparingInt(Person::getAge); - - @Override - public int compareTo(@NotNull Person o) { - return COMPARE_FIRST_LAST_NAME_AGE.compare(this, o); // Good: Comparator initialized once as static final field - } -} -]]> - - - Date: Wed, 17 Sep 2025 10:32:54 +0200 Subject: [PATCH 7/7] PMF: Cleanup and format --- .config/pmd/java/ruleset.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.config/pmd/java/ruleset.xml b/.config/pmd/java/ruleset.xml index a5b76341..748c826e 100644 --- a/.config/pmd/java/ruleset.xml +++ b/.config/pmd/java/ruleset.xml @@ -448,7 +448,7 @@ String good_replaceInnerLineBreakBySpace() { - + + 3 @@ -565,7 +565,7 @@ public enum Fruit { Solution: Usually a pattern is a literal, not dynamic and can be compiled only once. Assign it to a private static field. java.util.Pattern objects are thread-safe so they can be shared among threads. - + 2